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

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

Merged
merged 1 commit into from Sep 19, 2019

Conversation

@ClobberXD
Copy link
Contributor

commented Jul 15, 2018

This PR aims to make the zoom feature more generic by allowing mods to individually override player FOVs with arbitrary values. Once this is merged, I'll work on making builtin zoom use these methods. Here are the changes:

  • Add Lua API methods ObjectRef:set_fov(), ObjectRef:get_fov().
    • set_fov takes two params:
      • The FOV value.
      • A boolean to indicate whether the FOV value is a multiplier or not. Defaults to false - i.e. the value is an override by default.
    • get_fov returns a tuple containing the aforementioned data.
    • Passing 0 to set_fov will clear the FOV override.
  • Add PlayerFovSpec struct, to store f32 fov and bool is_multiplier, and add getter and setter.
  • Implement server->client FOV-sending using a new packet TOCLIENT_FOV.

Here's a mod to test out the new Lua API methods: fov_test. Check out the mod's README for the chat-commands' documentation.

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch from 1c67a1e to 672d65f Jul 15, 2018

@lhofhansl

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2018

How should be integrate this best with the zoom stuff?
Perhaps we simply allow to set the fov, and zoom is just a boolean controlling whether the server is allow to retrieve more distant blocks...?

@paramat FYI

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2018

That sounds like a good idea, since there are situations where only the fov change is required, and loading distant blocks is unnecessary...

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 15, 2018

That sounds like a good idea, since there are situations where only the fov change is required, and loading distant blocks is unnecessary...

I agree, you may want to set a very narrow FOV for an effect but not want world 2000 nodes away being generated and/or loaded.

@TMcSquared

This comment has been minimized.

Copy link

commented Jul 16, 2018

I just had a thought, someone could use this feature to animate the FOV for a nausea-type effect

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

Just a gentle reminder: The PR is still WIP since the documentation hasn't been updated yet, but the actual code is complete. I can start working on the documentation only when the code has been approved, and nothing needs to be majorly modified. :)

@rubenwardy rubenwardy changed the title [WIP] Expose player FOV to Lua API Expose player FOV to Lua API Jul 16, 2018

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch from 672d65f to d211afa Jul 25, 2018

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

Has anyone reviewed this PR yet? Is there anything that needs to be changed in a major way? I'll start adding the documentation now; if something needs change, will update the relevant docs accordingly...

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch from e965c60 to 84c12c2 Aug 4, 2018

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

Updated lua_api.txt, but I'm still not sure about whether or not to bump the latest protocol version. I'm sure the minimum version needn't be bumped since only new packets have been added, preserving backwards compat. with version 36.

@SmallJoker
Copy link
Member

left a comment

Couldn't this be just like the physics modifiers, so that the FOV is changed by a factor instead of overwriting the client's settings?

doc/lua_api.txt Outdated Show resolved Hide resolved
src/camera.cpp Outdated Show resolved Hide resolved
src/client.h Outdated Show resolved Hide resolved
src/network/clientopcodes.cpp Outdated Show resolved Hide resolved
src/network/clientpackethandler.cpp Outdated Show resolved Hide resolved
src/network/serveropcodes.cpp Outdated Show resolved Hide resolved
src/network/serveropcodes.cpp Outdated Show resolved Hide resolved
src/remoteplayer.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 Outdated Show resolved Hide resolved
@paramat

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Where is the option for controlling distant-world loading caused by zoom?

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

@paramat This does not cause distant-world loading at all. Only the FOV is changed. Should this behaviour be changed? If yes, how?

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch from 6b76c24 to 458fb8e Aug 7, 2018

@paramat

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

Ok i see. It's ok then. I see no need for this to cause distant world loading.

@SmallJoker
Copy link
Member

left a comment

Contains a bit too many irrelevant changes for my taste. Also needs some changes.

src/remoteplayer.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 Outdated Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch from a4ceb26 to 2935602 Aug 17, 2018

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch 3 times, most recently from e939669 to 9d6aff3 Aug 17, 2018

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

Addressed comments; rebased. What would be the future of zoom, keeping in mind this feature?

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch from ffe0842 to bc26d0e Aug 31, 2019

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Rebased. I've lost count of the number of times I've rebased this PR until now... :(

The incremental inv. sending commit already bumped the protocol version to 38, so this PR will bump it up to 39 instead.

builtin/game/features.lua Outdated Show resolved Hide resolved
src/client/camera.cpp Outdated Show resolved Hide resolved

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch from bc26d0e to 32f287b Aug 31, 2019

src/network/networkprotocol.h Outdated Show resolved Hide resolved
src/script/lua_api/l_object.cpp Outdated Show resolved Hide resolved

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch 3 times, most recently from e05c7ff to cec4943 Aug 31, 2019

@ClobberXD ClobberXD requested a review from SmallJoker Sep 11, 2019

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@sfan5 I've tested the new FOV checking order (see #7557 (comment)), and it works perfectly. I've used the fov_test mod linked in the first post. I've added another chat-command to the mod, to allow testers to verify MT's FOV checking order.

I've also made the Zoom currently disabled by game or mod status message appear when the server enforces a certain FOV, not allowing the client to use the zoom feature.

@SmallJoker Code has changed quite a bit, please re-review if possible. Thanks.

I'll also try to add a couple of unittests, if possible.

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch 2 times, most recently from 4ff7499 to c0caf51 Sep 11, 2019

@sfan5 sfan5 removed the Rebase needed label Sep 11, 2019

@ClobberXD ClobberXD requested a review from sfan5 Sep 14, 2019

@ClobberXD ClobberXD force-pushed the ClobberXD:lua_fov branch from c0caf51 to afb7558 Sep 14, 2019

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Rebased. Would be great to have this PR merged for 5.1.0

@SmallJoker
Copy link
Member

left a comment

LGTM

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

Hi, i removed the milestone back in May because that was soon after the release of 5.0.1 and milestones are (as i understand) primarily used when approaching a release to highlight PRs/issues that must be merged/attended to before release. Or, more rarely, features where the core devs have agreed that they really want the feature merged for the release.

I'm not keen on the casual use of milestones for saying 'it would be nice to have this merged for next release' because that is automatically the case for every PR, so is somewhat redundant. This use of milestones tends to result in them being too casually added in large numbers, only to be removed again when approaching the release and realising a merge probably won't happen in time. These often then get re-added as milesetones for the following release and it starts all over again. it just creates a redundant mess of labels.

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

@paramat Thanks for clarifying. But wasn't this PR agreed to be merged for 5.1.0? At least that's what nerzhul said. Wasn't this a joint decision?

@sfan5 sfan5 removed their request for review Sep 16, 2019

@sfan5
sfan5 approved these changes Sep 16, 2019
Copy link
Member

left a comment

Works.

@sfan5 sfan5 added >= Two approvals and removed One approval labels Sep 16, 2019

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

A PR is not agreed to be merged until it has 2 approvals, however, a few of us may have agreed it would be good to merge it for 5.1.0 if possible. However, nerzhul added this to the milestone in Dec 2018 which was long before 5.0.0, that's too far off to decide it will be merged for 5.1.0, which is why i removed the milestone, and therefore i expect he did that casually and alone.
If we had agreed we really wanted this in 5.1.0 the milestone would have been re-added, maybe together with a comment.

@ClobberXD

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

Alright, makes sense. Thanks again for clearing up things. :)

@sfan5 sfan5 merged commit 47da640 into minetest:master Sep 19, 2019

@ClobberXD ClobberXD deleted the ClobberXD:lua_fov branch Sep 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.