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

bigfoot547 · 2017-05-22T03:26:18Z

No description provided.

nerzhul · 2017-05-22T05:31:06Z

Thanks but you missed the feature merge window, it will be delayed on next release

bigfoot547 · 2017-05-22T16:22:29Z

I understand. :'(

nerzhul · 2017-05-26T17:19:24Z

doc/client_lua_api.md

-    * Act as if `message` was typed by the player into the terminal.
-* `minetest.run_server_chatcommand(cmd, param)`
-    * Alias for `minetest.send_chat_message("/" .. cmd .. " " .. param)`
-* `minetest.clear_out_chat_queue()`


why remove this ?

@nerzhul It was a mistake when copying stuff... I will fix it in a future commit.

nerzhul · 2017-05-26T17:19:40Z

src/camera.h

+		return m_cache_fov;
+	}
+
+	inline f32 getZoomFov()


all getter should be const functions

SmallJoker · 2017-05-27T08:18:33Z

What about the zoom feature we've already got? This PR in combination with a key press getter function will make the required privilege superflous.

Desour · 2017-06-01T16:11:41Z

@SmallJoker The minimum value of zoom fov is much lower than the one of the normal fov.
Or aren't the added functions limited? That would be great IMO!

nerzhul · 2017-06-03T21:12:43Z

Okay for me

SmallJoker · 2017-06-04T09:27:28Z

src/camera.h

+	inline void setFov(f32 fov)
+	{
+		m_cache_fov = fov;
+	}


Quite nonsense to have getter and setter functions for m_cache_fov and m_cache_zoom_fov. If there were boundary checks, this would be acceptable but how it is right now you could make them public as cache_zoom.

nerzhul · 2017-06-04T12:12:22Z

Please let members private it's an object not a structure.

bigfoot547 · 2017-06-04T13:53:55Z

@SmallJoker the range is limited because of this, right here.

SmallJoker · 2017-06-04T14:32:21Z

@nerzhul Well, we already have public members for objects. The best example for this is the Player class.

@bigfoot547 I'm talking about limits when setting the cache_fov/cache_zoom_fov variables to a new value. The range limit is done in each Camera::update call and does not affect the saved settings at all. That's why I would prefer to expose this variable as public instead of adding functions.

nerzhul · 2017-06-04T16:44:22Z

@SmallJoker yes i know we have that ugly interface in player; i started to cleaning up years ago by moving some attributes to PlayerSAO/RemotePlayer or LocalPlayer for client, but not finished yet. Having getter and setters has no performance impact on code (they are inlined in majority cases), and we respect properly the difference between a struct and a class (class intend to have private members and struct public members)

nerzhul · 2017-06-10T13:39:46Z

doc/client_lua_api.md

@@ -842,16 +842,25 @@ Please do not try to access the reference until the camera is initialized, other
    * Returns with same syntax as above
 * `get_fov()`
    * Returns:
-
+    


can you fix those spaces ?

ShadowNinja · 2017-06-10T17:22:41Z

doc/client_lua_api.md

+* `save_fov()`
+	* Sets the "fov" setting to `get_fov().actual`
+* `save_zoom_fov()`
+	* Sets the "zoom_fov" setting to `get_fov().zoom`


Just add a persist boolean argument to the FOV setters.

paramat · 2017-06-17T18:18:55Z

A description in the first post would be nice, makes review easier.

paramat · 2017-06-20T00:21:08Z

SmallJoker wrote:

What about the zoom feature we've already got? This PR in combination with a key press getter function will make the required privilege superflous.

@SmallJoker did you get a response to this?
Since this is CSM, altering FOV must surely need the 'zoom' priviledge? The current Z key zoom is clientside which is why a priviledge was added for that.

The zoom priviledge needs discussing first.
If the 'zoom' priv is not checked in this PR these functions would need to be restricted by the flavour PR @nerzhul .

red-001 · 2017-06-20T00:41:51Z

why do we even have a zoom privilege in the first place?

paramat · 2017-06-20T01:14:48Z

See the PR thread that added zoom. Zoom adds a 'superpower' to a player so a restriction was required for survival such that a player would need to craft a telescope type item before being able to zoom.

bigfoot547 · 2017-07-17T19:02:22Z

What's wrong with the current implementation?

paramat · 2017-07-17T19:55:21Z

Sorry didn't see the line above, seems ok then.

bigfoot547 · 2017-07-17T19:55:42Z

Thanks @paramat

paramat · 2017-07-17T20:05:55Z

doc/client_lua_api.md

@@ -841,17 +841,25 @@ Please do not try to access the reference until the camera is initialized, other
 * `get_camera_mode()`
    * Returns with same syntax as above
 * `get_fov()`
+	* All values are in degrees


Indentation should match other lines in this section.

paramat · 2017-07-17T20:06:03Z

doc/client_lua_api.md

+	* Pass true to the second argument to save the zoom fov to the setting.
+* `set_zoom_fov(fov_degrees, [persist])`
+	* Sets the FOV for the local player when zoomed in
+	* Pass true to the second argument to save the zoom fov to the setting.


Indentation should match other lines in this section.

paramat · 2017-07-17T20:07:57Z

doc/client_lua_api.md

+         y = float,        -- Vertical FOV
+         max = float,      -- The maximum of x and y
+         actual = integer, -- What the FOV of the player would be (not adjusted for zooming)
+         zoom = integer    -- What the FOV of the player when zooming would be


What's the meaning of 'would be' in these lines? How is it different from x,y? This needs clarifying.

bigfoot547 · 2017-09-29T00:27:37Z

@paramat: I have rebased and addressed your reviews, so can you please review again?

Conflicts: src/camera.h src/script/lua_api/l_camera.cpp

bigfoot547 · 2017-11-29T22:23:01Z

needs a rebase again...

bigfoot547 · 2017-11-29T22:33:26Z

Rebased. @SmallJoker @paramat Please review. I don't want this PR to sit doing nothing :L

rubenwardy

should be added to CSM flavours.

bigfoot547 · 2017-11-29T22:45:58Z

Ok, I'll look into it

paramat · 2017-11-30T00:00:04Z

👎 nerzhul this can't be merged because it conflicts with the zoomFOV changes being made by lhofhansl and me, and has errors.
See #6650 , this PR is low priority and should wait until that is merged, and then reworked around the new approach.

In this PR camera FOV is now restricted to 7 to 160 when it needs to range from 1 to 160.
Per-player ZoomFOV has to be overridable by the server when it is set by #6650
Client-settable ZoomFOV must be limited to a minimum of 15 degrees as in #6650 , because zoom now loads distant terrain which puts a load on the server and gives the player an advantage of being able to generate and load terrain beyond the server's maximum generate and send ranges.
Client-settable zoom should not affect the server and cause it to generate or load terrain beyond these ranges.

Is this useful for server-provided CSM? That is our new decider on CSM features, as CSM has to be as minimal as possible. If it is, it needs to be controlled by flavors as it is something that gives a player an advantage: if the client has distant mapblocks loaded, it can look at those and magnify them.

paramat · 2017-12-02T05:00:01Z

👎 Actually, sorry but the zoom FOV parts of this can't be merged in any form, because zoom now loads and/or generates distant world like a telescope, a powerful ability that must be under complete server control and shouldn't be added to CSM. It also conflicts with the intentions of recent zoom changes by lhofhansl and myself.

If you split off the normal FOV feature that might be fine.
A description in the first post is needed, it's not a simple or trivial change so it should have a clear summary. I've asked twice before, you may not get reviews or attention without a description.

bigfoot547 · 2017-12-02T14:23:23Z

:/ well, I guess there's only one thing to say.
https://youtube.com/watch?v=cdEQmpVIE4A

paramat added @ Script API Feature ✨ PRs that add or enhance a feature labels May 22, 2017

bigfoot547 force-pushed the csm-set-fov branch from 8c7fff0 to 1832cdc Compare May 22, 2017 22:44

nerzhul added this to Feature PR in Minetest 5.0.0 blockers May 23, 2017

nerzhul added this to the 0.5.0 milestone May 26, 2017

nerzhul requested changes May 26, 2017

View reviewed changes

bigfoot547 force-pushed the csm-set-fov branch from adfe081 to 670abb8 Compare June 3, 2017 21:09

nerzhul approved these changes Jun 3, 2017

View reviewed changes

nerzhul added the One approval ✅ ◻️ label Jun 3, 2017

nerzhul moved this from Feature PR to Ready to merge in Minetest 5.0.0 blockers Jun 4, 2017

SmallJoker reviewed Jun 4, 2017

View reviewed changes

nerzhul reviewed Jun 10, 2017

View reviewed changes

ShadowNinja reviewed Jun 10, 2017

View reviewed changes

ShadowNinja moved this from Ready to merge to Feature PR in Minetest 5.0.0 blockers Jun 17, 2017

nerzhul moved this from Feature PR to Ready to merge in Minetest 5.0.0 blockers Jun 17, 2017

nerzhul moved this from Ready to merge to Feature PR in Minetest 5.0.0 blockers Jun 17, 2017

bigfoot547 force-pushed the csm-set-fov branch from 3500a03 to fd347f9 Compare June 19, 2017 00:09

paramat reviewed Jul 17, 2017

View reviewed changes

Addressed @paramat

b5a2c8b

Merge remote-tracking branch 'minetest/master' into csm-set-fov

62efdfb

Conflicts: src/camera.h src/script/lua_api/l_camera.cpp

Fix conflicts?

27d6083

nerzhul approved these changes Nov 29, 2017

View reviewed changes

rubenwardy requested changes Nov 29, 2017

View reviewed changes

paramat added Action / change needed Code still needs changes (PR) / more information requested (Issues) @ Client Script API Low priority labels Nov 30, 2017

ThomasMonroe314 mentioned this pull request Dec 1, 2017

Add more settings to the pause menu. #6722

Open

bigfoot547 closed this Dec 2, 2017

nerzhul moved this from Feature PR to Done in Minetest 5.0.0 blockers Jan 4, 2018

ClobberXD mentioned this pull request Dec 7, 2018

Add support for per-player FOV overrides and multipliers #7557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bigfoot547 commented May 22, 2017

nerzhul commented May 22, 2017

bigfoot547 commented May 22, 2017

nerzhul May 26, 2017

bigfoot547 May 26, 2017

nerzhul May 26, 2017

SmallJoker commented May 27, 2017

Desour commented Jun 1, 2017

nerzhul commented Jun 3, 2017

SmallJoker Jun 4, 2017

nerzhul commented Jun 4, 2017 via email

bigfoot547 commented Jun 4, 2017

SmallJoker commented Jun 4, 2017

nerzhul commented Jun 4, 2017

nerzhul Jun 10, 2017

ShadowNinja Jun 10, 2017

bigfoot547 Jun 19, 2017

paramat commented Jun 17, 2017 •

edited

paramat commented Jun 20, 2017 •

edited

red-001 commented Jun 20, 2017

paramat commented Jun 20, 2017

bigfoot547 commented Jul 17, 2017

paramat commented Jul 17, 2017

bigfoot547 commented Jul 17, 2017

paramat Jul 17, 2017

paramat Jul 17, 2017

paramat Jul 17, 2017

bigfoot547 commented Sep 29, 2017

bigfoot547 commented Nov 29, 2017

bigfoot547 commented Nov 29, 2017

rubenwardy left a comment

bigfoot547 commented Nov 29, 2017

paramat commented Nov 30, 2017 •

edited

paramat commented Dec 2, 2017 •

edited

bigfoot547 commented Dec 2, 2017

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

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

Conversation

bigfoot547 commented May 22, 2017

nerzhul commented May 22, 2017

bigfoot547 commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmallJoker commented May 27, 2017

Desour commented Jun 1, 2017

nerzhul commented Jun 3, 2017

Choose a reason for hiding this comment

nerzhul commented Jun 4, 2017 via email

bigfoot547 commented Jun 4, 2017

SmallJoker commented Jun 4, 2017

nerzhul commented Jun 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramat commented Jun 17, 2017 • edited

paramat commented Jun 20, 2017 • edited

red-001 commented Jun 20, 2017

paramat commented Jun 20, 2017

bigfoot547 commented Jul 17, 2017

paramat commented Jul 17, 2017

bigfoot547 commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigfoot547 commented Sep 29, 2017

bigfoot547 commented Nov 29, 2017

bigfoot547 commented Nov 29, 2017

rubenwardy left a comment

Choose a reason for hiding this comment

bigfoot547 commented Nov 29, 2017

paramat commented Nov 30, 2017 • edited

paramat commented Dec 2, 2017 • edited

bigfoot547 commented Dec 2, 2017

paramat commented Jun 17, 2017 •

edited

paramat commented Jun 20, 2017 •

edited

paramat commented Nov 30, 2017 •

edited

paramat commented Dec 2, 2017 •

edited