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

Optimize/adjust blocks/ActiveObjects sent at the server based on client settings. #4811

Merged
merged 4 commits into from Nov 30, 2016

Conversation

@lhofhansl
Copy link
Contributor

commented Nov 26, 2016

I expect this to stir some controversy. Took me a while to make this change, so have at least a look :)

This sends the client's fov and view range to the server. The server will adjust the blocks sent (both due to fov and the range the client chose).

The upshot is:

  1. the fov the server uses is always correct (no strange guesses or 4/3 adjustment needed)
  2. the server will only send blocks that the client can actually see (up to max_block_send_distance)
  3. server will only send active objects that the client can see (up to active_object_send_range_blocks)
  4. server will only generate blocks that the client can see (up to max_block_generate_distance)
    (5. the view range change will only take effect when the player moves next, by itself it does not cause a roundtrip)

This is quite a performance improvement if the client is using a smaller view range. And it's client specific. I'd expect busy servers will see their load reduced a lot.

Now I had to change stuff at a bunch of places. This work fine (have been playing with this with my son :), but this needs a very careful review as a bunch of the areas I touched are new to me.

@rubenwardy @sfan5 @est31 related to #4808

if (m_draw_control.range_all) {
m_cameranode->setFarValue(100000.0);
return;
}

This comment has been minimized.

Copy link
@lhofhansl

lhofhansl Nov 26, 2016

Author Contributor

This is do that view range get's updated even while fog is turned off. No perf detriment. Makes it nicer to change the view range to retrieve more (or less) data from the server, while seeing what's going on.

This comment has been minimized.

Copy link
@paramat

paramat Nov 28, 2016

Member

But does this affect the ability to view all loaded blocks, out to any distance, with the 'R' key?
'R' is very often used to view all loaded blocks out to extremely long distances, it is not meant to just turn fog off.

This comment has been minimized.

Copy link
@paramat

paramat Nov 28, 2016

Member

Yeah i'm fairly sure this is not a good idea.
EDIT I misunderstood, see below.

This comment has been minimized.

Copy link
@paramat

paramat Nov 28, 2016

Member

Key 'F3' is for toggling fog.
m_draw_control.range_all is the 'R' key.

This comment has been minimized.

Copy link
@lhofhansl

lhofhansl Nov 28, 2016

Author Contributor

'R' will still show all loaded blocks. Here I'm just allowing changing the view-range while range_all is active. There is no functional change (before pressing + or - while range_all was active had no effect - even though it would tell you on the screen).

I see your point on 'F3', though. I think what you are saying is that with range_all we should always request all blocks from the server. Is that what you are saying, i.e. with range_all _always retrieve max_block_send_distance range of blocks from the server?

That change would have to be made in class Client.

I do find it a but too easy then to pull the maximum amount of data from the server. As it is now you'd have to select range_all and increase the view_range to get up to max_block_send_distance worth of blocks.

Don't feel strongly one way or the other. Tell me what you prefer and I'll do that. :)

This comment has been minimized.

Copy link
@paramat

paramat Nov 28, 2016

Member

Ok sorry i misunderstood, this is fine then.

This comment has been minimized.

Copy link
@paramat

paramat Nov 28, 2016

Member

Is that what you are saying, i.e. with range_all _always retrieve max_block_send_distance range of blocks from the server?

No that's not what i meant, sorry.

As it is now you'd have to select range_all and increase the view_range to get up to max_block_send_distance worth of blocks.

This is fine, as it is in this first commit.

This comment has been minimized.

Copy link
@paramat

paramat Nov 28, 2016

Member

When using 'R' there is no need to automatically start fetching all blocks out to max_block_send_distance.

src/client.cpp Outdated
*pkt << position << speed << pitch << yaw << keyPressed;
*pkt << fov << v_range;

This comment has been minimized.

Copy link
@lhofhansl

lhofhansl Nov 26, 2016

Author Contributor

Can I do this and not break wire compatibility? I think a newer server can work with an older client, but not necessarily a new client with an old server - the server would not drain the packet... Not sure.

This comment has been minimized.

Copy link
@est31

est31 Nov 26, 2016

Contributor

This will work, yes, but you need to initialize the value in the reading part, see my comment.

src/client.cpp Outdated
@@ -948,9 +950,11 @@ void writePlayerPos(LocalPlayer *myplayer, NetworkPacket *pkt)
[12+12] s32 pitch*100
[12+12+4] s32 yaw*100
[12+12+4+4] u32 keyPressed
[12+12+4+4+4] s32 fov*100

This comment has been minimized.

Copy link
@est31

est31 Nov 26, 2016

Contributor

don't forget to update src/network/networkprotocol.h

This comment has been minimized.

Copy link
@lhofhansl

lhofhansl Nov 26, 2016

Author Contributor

done

@est31
Copy link
Contributor

left a comment

The change looks good code wise except the things noted. Concept wise I'm not sure whether its a good idea to send the data each time the player moves (which is very often)... In the best case the info is only sent at initial connect. Maybe we should really adopt a system where we can exchange generic settings... Any better ideas?

src/network/serverpackethandler.cpp Outdated

if (pkt->getRemainingBytes() >= 4)
*pkt >> keyPressed;
if (pkt->getRemainingBytes() >= 4) {
*pkt >> f32fov;

This comment has been minimized.

Copy link
@est31

est31 Nov 26, 2016

Contributor

use tabs only

src/network/serverpackethandler.cpp Outdated
@@ -791,9 +791,17 @@ void Server::process_PlayerPos(RemotePlayer *player, PlayerSAO *playersao,
f32 pitch = (f32)f32pitch / 100.0;
f32 yaw = (f32)f32yaw / 100.0;
u32 keyPressed = 0;
f32 fov = 72;
s32 v_range;

This comment has been minimized.

Copy link
@est31

est31 Nov 26, 2016

Contributor

this is not initialized

@lhofhansl lhofhansl force-pushed the lhofhansl:range_adjust branch 2 times, most recently Nov 26, 2016

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

@est31 It's sent along when position data is sent anyway, no extra network roundtrips. Are you concerned about the extra bytes sent? We'll save way more in traffic from the server to the client.

The fov only needs to be sent once since the client cannot (right now) change it in game. The view range needs to be sent each time.

@est31

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2016

Yes, I guess there will be much less traffic, but the parameter changes very rarely so it makes little sense to send it in a packet that gets sent so often.

I'm just concerned whether there is a better place to put this instead :)

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

Yeah not sure either :)
For the view range that seems to be the right place (it can be changed in game - I change it all the time, but that's just me). fov could be sent on connecting only - somehow. Conceptually both are part of what the client sees (same as camera pitch and yaw and the players position, etc), so it does not seem completely out of place.

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

I suppose we could add Server::handleCommand_fov and void Server::handleCommand_v_range, but that seems overkill too.

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

One bug I found: In a new world neither fov not v_range are initialized, so until a player moves the screen's just gray.

@lhofhansl lhofhansl force-pushed the lhofhansl:range_adjust branch Nov 26, 2016

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

Fixed now. When the world was just created and before the player moves at all, we need to have good defaults. Playing with the view range is quite fun (even when full range is on).

I also tried with an old client on a new server.

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

Note that with the default setting of view range = 100 this saves 1 - 6^3/9^3 = 70% of block traffic from the server, actually reducing the lag at the corners as the server has less work to do.

(I'll shut up now, until somebody looks :) )

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 26, 2016

Interesting, currently of course all blocks up to max_block_send_distance (160 nodes) are sent to the client.
And yes players tend to change their view range very often, every few minutes for me.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 26, 2016

server will only send active blocks that the client can see (up to active_object_send_range_blocks)

'can see' does this mean within FOV? ABMs should always run in the defined radius all around the player, the player's view is irrelevant here i feel. The defined radius is usually fairly small at 2 blocks and very few players will have a view range smaller than 2 blocks.

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

@paramat There are two parts to this.

  1. fov makes sure that the server uses the client's fov rather than a hardcoded 70 degrees.
  2. view range limits the number of blocks sent and the range in which ABMs run.

So ABMs run within the view range of the client, but are not subject to fov. It always bugged me that ABMs suddenly disappear if active_object_send_range_blocks is too small, or waste resources if it's too large (and slow my game down).

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 26, 2016

Ok so

server will only send active blocks that the client can see

Actually means "server will only run ABMS in blocks that the client can see". That's what confused me.

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

Ah right. That's wasn't stated clearly. Correct.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 26, 2016

Ok it's ABMs. Deleted my previous comment.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 27, 2016

server will only send active blocks that the client can see (up to active_object_send_range_blocks)

Just to avoid confusion, i think you mean "(up to active_block_range)":

#    How large area of blocks are subject to the active block stuff, stated in mapblocks (16 nodes).
#    In active blocks objects are loaded and ABMs run.
#    type: int
# active_block_range = 2
@paramat

This comment has been minimized.

Copy link
Member

commented Nov 27, 2016

I like most of this and it has a lot of potential.

I'm unsure about bothering with ABMs, as 'active block range' is almost always set low (2 or 3). If it was set higher that would be a huge and very noticeable load on the server and would be corrected quickly.
Very few clients have a view range less than 2 or 3 blocks so this wouldn't have much affect anyway.
I feel that the server should decide what range around players to run ABMs in, instead of it being dictated by individual clients view range. Running ABMs is a 'world thing' and i feel should be independent of client's view.

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2016

It's active_object_send_range_blocks (see server.cpp).

If it's always small then this PR doesn't do anything, it will never increase the number of active objects. If, on the other hand, active_object_send_range_blocks is larger (so that mobs don't disappear on you) then this helps a lot.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 27, 2016

Now i'm confused.

#    From how far clients know about objects, stated in mapblocks (16 nodes).
#    type: int
# active_object_send_range_blocks = 3

#    How large area of blocks are subject to the active block stuff, stated in mapblocks (16 nodes).
#    In active blocks objects are loaded and ABMs run.
#    type: int
# active_block_range = 2

So you are referring to 'active objects' like mobs, being sent to the client, not ABMs. ABMs are things like grass growing on dirt etc.
Perhaps there's some interaction between the 2 though.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 27, 2016

server will only send active blocks that the client can see (up to active_object_send_range_blocks)

Maybe you mean "server will only send active objects that the client can see (up to active_object_send_range_blocks)"? Which seems fine.

However my opinion about ABMs is unchanged and is stated above.

@est31

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2016

So if I'd add 2 u16 fields to the player pos packet and send those when anything had changed, is everybody OK with that compromise?

Yes I can agree with that

@celeron55

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

Well, I was thinking u8 could hold the FOV angle in degrees (0-180?) just fine. Or really, nothing prevents just scaling whatever range is used to whatever bits are available; i.e. just make 0=0 and pi=255. It's quite common at least in embedded stuff and obviously does work.

I do know that MT is quite wasteful in many places but it's not really a good argument for being even more wasteful. 8)

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2016

@celeron55 :)
The camera reports it in rad. But sure, can translate back to degrees before sending and then back to rad after.

I'll try u8, u8 :)

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2016

Or I can scale rad by 80 rather than 100 :)

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2016

Will do this later today.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

If view range is u8 in mapblocks that's a max of 4096 nodes, which is more than enough, u8 in nodes is obviously not enough.

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2016

Yep, that's my plan: FOV in rad scaled by 80, view_range in map blocks.

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2016

Tested also with an old client on a new server, server correctly falls back to old behavior.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

I like that this is now using 2 u8s.

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2016

👍

@Zeno- Zeno- added >= Two approvals and removed One approval labels Nov 30, 2016

@Zeno- Zeno- merged commit 5dc6198 into minetest:master Nov 30, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@fwhcat

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2016

Nice work to all and thanks @lhofhansl

@lhofhansl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2016

Pffeewww. 122 comments later :)
Seriously, thanks for all the discussion here. Learned a lot about Minetest.

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.