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
Prefer loading blocks near predicted future player position #6447
Conversation
It's funny because I was using fast mode earlier and observing these islands you mention, and considered at the time that this was by design! Ps. It should be noted that this only occurs when noclip mode is on, because otherwise the player stops at current mapblocks boundary before the next is loaded. |
Addresses #1442 |
src/clientiface.cpp
Outdated
// Predict to next block | ||
v3f playerpos_predicted = playerpos + playerspeeddir*MAP_BLOCKSIZE*BS; | ||
f32 playerspeedlen = playerspeed.getLength(); | ||
if(playerspeedlen > 1.0*BS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces around '*'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.0f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space after 'if'
src/clientiface.cpp
Outdated
|
||
// Predict where player will be soon, load blocks around there first | ||
v3f playerpos_predicted = playerpos + playerspeeddir * | ||
MYMIN(playerspeedlen * BS, d_blocks_in_sight*0.5f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces around '*'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'd_blocks_in_sight' is in units of nodes or mapblocks? needs to be multiplied by BS or MAP_BLOCKSIZE * BS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So BS can then be factored out if present in all terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d_blocks_in_sight is already scaled correctly (in nodes, i.e. multiplied by MAP_BLOCKSIZE * BS), it is used later in the code (I just moved this code up).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
src/clientiface.cpp
Outdated
float camera_fov = sao->getFov(); | ||
// if FOV, wanted_range are not available (old client), fall back to old default | ||
if (wanted_range <= 0) wanted_range = 1000; | ||
if (camera_fov <= 0) camera_fov = (72.0*M_PI/180) * 4./3.; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces around '*' and '/'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State all numbers as floats
0.0f
72.0f
180.0f
4.0f
3.0f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanted_range =
and camera_fov =
belong each into a separate line.
EDIT: Even if this is only a code movement, it should be cleaned up when touching it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, missed that, agreed.
Excellent idea and much wanted.
otherwise for all speeds < 16 nodes per second (almost all speeds) this PR is reducing the predictive block loading. Also, since block loading acts in blocks there is no point in using a prediction distance smaller than 1 mapblock, because that would often result in no predictive action at all. |
src/clientiface.cpp
Outdated
s16 wanted_range = sao->getWantedRange(); | ||
float camera_fov = sao->getFov(); | ||
// if FOV, wanted_range are not available (old client), fall back to old default | ||
if (wanted_range <= 0) wanted_range = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't inline conditions
src/clientiface.cpp
Outdated
float camera_fov = sao->getFov(); | ||
// if FOV, wanted_range are not available (old client), fall back to old default | ||
if (wanted_range <= 0) wanted_range = 1000; | ||
if (camera_fov <= 0) camera_fov = (72.0*M_PI/180) * 4./3.; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compare float with floats and use the .f marker to prevent using doubles
src/clientiface.cpp
Outdated
if (wanted_range <= 0) wanted_range = 1000; | ||
if (camera_fov <= 0) camera_fov = (72.0*M_PI/180) * 4./3.; | ||
|
||
const s16 full_d_max = MYMIN(g_settings->getS16("max_block_send_distance"), wanted_range); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::min instead of MYMIN
I hope this makes it into the engine, it will help with the issue mentioned in #1442. It's very timely also, considering that I have just discovered that my players are taking advantage of this when dropping hundreds of blocks from cavern ceilings. (They point the camera up and are able to reach the floor without dying.) |
Couple of comments:
|
Not sure it's going to help with players not dying when falling from great height. |
Update addressing review comments. |
I think there's more that can be done. For example at high velocity we could reduce the fov of the retrieval cone. The effect would be that server and client would be able to better keep generating and retrieving blocks along the axis of movements, and not load the sides (which would soon not be visible anyway - we're moving). In fact, maybe that's a better solution altogether...? Not trying to predict the future position, but instead predict a good fov, so that blocks in the smaller cone are loaded faster. I'll try that too. In any case, if someone could test what I have now, that'd be cool. |
Arrghhh... Lemme fix the other MYMINs as well. Not sure why I did not see them. |
Tried the fov adjustment, but it's no good since one does not have to look into the direction of the movement (can move forward, but look down, in which case this is weird). |
There's one thing I noticed: If we do not predict the player's position, the player will have moved on before some of the close by blocks could be retrieved. That means now with the prediction client and server will likely render more blocks. This is apparent when you "fly" around in some altitude and have a mod that loads blocks somewhat slowly. There's no free lunch. :( |
src/clientiface.cpp
Outdated
s16 wanted_range = sao->getWantedRange(); | ||
float camera_fov = sao->getFov(); | ||
|
||
// if FOV, wanted_range are not available (old client), fall back to old default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're breaking compat with old clients now, so old client support can be dropped.
Now you're using std::min(), |
Compiled and ran it, the compile should have failed with loaded from somewhere. Anyway it's nice to be included where it is needed. |
Tangentially related... How much do we generally want to trust the client? |
OK. Done. Also reduce the effective blockrange sent in one roundtrip to 1 (instead of 2). |
No need, we're breaking compat in code inside each PR as we go. |
src/clientiface.cpp
Outdated
s16 wanted_range = sao->getWantedRange(); | ||
float camera_fov = sao->getFov(); | ||
|
||
const s16 full_d_max = std::min(g_settings->getS16("max_block_send_distance"), wanted_range); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
g_settings->getS16("max_block_send_distance") and the other variable should be cached somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the code that loads blocks, and sends them over the network, i.e. not hot. This seems overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also surely it being 'const' means it is as good as cached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const means only we cannot modified it, its for dev, not for runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about 'static const'? Since these settings are not changed often.
Maybe i misunderstand 'const' and 'static const' i thought these were only calculated once per game session or program session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry indeed these can't be static const since there is a min with wanted range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is possible but i would cache the actual g_settings->get()
values as private members of class RemoteClient
and initialise them in the constructor?
They don't need to be refreshed because they are settings not often changed by a user, we actually have many settings that only take effect on a program restart, for performance reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However it's not high priority so i might still +1 if you do not optimise, but i would prefer you do if it is possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really do not think that is necessary. There are many more configs that we retrieve this way - not an excuse not to fix it, but in this code it is simply not needed and will not help with code readability.
We could have a general PR to think about caching of configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i won't insist on it.
As little as possible due to hacked clients, which are a big problem for server owners. Minetest's general approach is that the server is in control of as much as reasonably possible. |
src/clientiface.cpp
Outdated
// Predict where player will be soon, load blocks around there first | ||
// We use the client-reported speed as an indicator, but do not move the | ||
// center point further than 1/2 the visible range away | ||
v3f playerpos_predicted = playerpos + playerspeeddir * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about optimising this by moving this calculation into the condition above so that it only occurs when player speed > 1 * BS?
v3f playerpos_predicted = playerpos;
if (playerspeedlen > 1.0f * BS) {
v3f playerspeeddir = playerspeed / playerspeedlen;
playerpos_predicted += playerspeeddir * rangelim(playerspeedlen * BS, MAP_BLOCKSIZE * BS, d_blocks_in_sight * 0.5f);
}
? and delete line 104.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimizing is not needed here.
You mean for readability...? There I would agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's not hot code, but we still optimise code everywhere as these small optimisations do add up, over time this helps to make MT more lightweight. So in my opinion there is no such thing as 'optimising is not needed here'.
Thanks for doing this.
Is it possible for somebody else to test this and share the experience? |
@lhofhansl, I tested it and it seems to be fine. I have made a video that includes me using fast move without the pull request and with the pull request. I cloned the newest version of the Minetest repository, so I'm using what will be 5.0: https://streamable.com/cu4uj If anyone sees this within the next few minutes, then streamable is still uploading. The video timeline is as such: MV forward without PR -> Jump down hole without PR -> MV forward with PR -> Jump down hole with PR. There are some transition titles so you know what I'm doing: Forgive my video editing skills (for some reason that robot voice won't stay out of my videos). Moving with the pull request seems to be faster, meaning the terrain generates in front of me faster. In the video I get into terrain I did not generate yet, so it lags a little, just compare it to the first part of the first video. I'm using the default view distance of 100, and also take note that I'm not at the exact same height when moving forward in both parts of that video. The best part about this though is that when falling straight down I don't hit the ignore nodes with your pull request. In the video, you can see me hit the ignore nodes when I do not have your pull request. When playing Minetest, hitting those invisible ignore nodes is frustrating and it makes the game seem buggier than it is; this will make Minetest seem so much more refined! I'm very glad this is being solved and implemented. Someone should tell @Wuzzy2 to check this out. Maybe we can close #1442. |
Wuzzy2
I welcome my mistake being pointed ouot, but it's just a minor mistake and no big deal as you imply with 'the kind of nonsense which must be called out immediately'.
No the response was not at all appropriate, you can't justify it, i'll assume you're angry at me for some reason. It seems that when you make a minor mistake you want people to swear, write 'yet another mistake, you're supposed to be a contributor' :) I couldn't do that. To be clear i do not want him removed from the organisation, he is/was a good contributor. |
All are in Irrlicht units it seems, so flying at 20n/s prediction is 40 nodes ahead (2.5 mapblocks), Falling at 100n/s, the predicted point will be 200 nodes below, wanted range is increased by 100 nodes, and block-loading FOV is divided by 3. Seems reasonable. When walking, predicted point 8 nodes ahead, so half the time there is no prediction, not a big issue. It would be good to test this on an actual multiplayer server, i feel my singleplayer tests are not enough. |
There's some more tweaks I want to try:
I wonder whether I'm doing too many updates on this PR - showing the process and my thinking for discussion... Or whether it'd be better to just post a PR that I think is mostly finished...? |
It's fine. Please also extend the first comment (PR description). |
So hold off on testing this. Some more changes coming. :) |
FOV adjustment based on speed was rejected in minetest (#385), it was merged in freeminer and disabled by default if memory serves. When the player goes by cart or is attached to some other moving object, mapblocks in the direction of the velocity of the player don't need to be loaded earlier because the cart collision is calculated server-side, do they? |
Yes, they do. It kinda defeats the point of the PR if they aren't loaded (and sent to client) ahead of the player while riding a cart. Also, player is most frequently moving fast when riding some vehicle (if they aren't falling). |
@lhofhansl if I may suggest, the direction of player movement should always override the direction of the view cone, if they are contradictory and the player is moving faster than normal. Without this PR, riding a cart on a server with a slow network is annoying because the cart keeps flying off into the grey. |
@HybridDog Yeah, client side FOV adjustment looks weird, I tried (see last message) and came to the same conclusion. This PR does it server side fov adjustment, since the faster you move the faster blocks "fall out" on the sides and hence we'd better focus on loading more blocks in the viewing/moving direction. @BluebirdGreycoat Not quite. The player can move forward and look up or down, or look forward and move backwards. In that case it is quite important to check for the viewing direction. |
Yes, I'm thinking that if the player is moving sufficiently fast then the viewing cone should be ignored. Otherwise, a falling player could prevent blocks below them from loading simply by looking upwards, which is the current situation anyway. I also think that the speed at which the viewing cone should be ignored should be configurable per-server, since no set speed is going to work for all servers. For example, a server could have carts go at a speed just lower than 'fast' and thus might want to lower the limit for ignoring the viewing cone. Edit: the predicted future position doesn't need to move very far for this to work. It only needs to move ahead the distance of the server's configured block send range, say. |
Looked at #385. Note that this is the opposite! I am decreasing the FOV (server side only, so no client jump) when moving forward, since blocks on the side will disappear quickly anyway. |
Not enforcing the view-cone is tricky. Which blocks should I retrieve from behind the player? All? That'd be a lot of blocks potentially. Could try to move shift the view-cone. Lemme think on that. |
A simple if {} can fix that. If player is falling but view cone is pointed elsewhere, just retrieve blocks directly below the player (prioritize them above the blocks the player is looking at). If player is moving sideways but looking elsewhere, same thing, just in the X and Z directions. After all if the player isn't looking in the direction they're moving, the reason we'd be retrieving blocks is to allow player movement to continue uninterrupted as much as possible, so there'd be no point in retrieving more blocks than that. Edit: 90% of the time, player is looking in same direction they move so I'm just proposing doing something different for the 10% of the time they spend falling (or sitting in a cart and looking back). |
@Wuzzy2 you're the only person who has consistently thumbed-up the abuse in this thread, i'm surprised and disappointed. |
I really don't like this effect, unfortunately because MC uses it (the opposite infact) many think it's a good idea, it's visually disorientating and nauseating.
Good idea.
The block-loading FOV? Yeah sounds good. 20n/s is a fairly universal top speed for players or admin, reducing block FOV to half in this situation may be acceptable.
I have been thinking about this too, currently falling damage can be reduced by simply looking upwards to cause many unloaded block collisions (see the falling issue). This is interesting and appreciated work, i will test what you come up with. |
BluebirdGreycoat, it doesn't make sense to me, why does the player have to have the mapblock that the cart can drive there although real object movement is done on the server? |
@HybridDog there's too distinct cases here. In the case of falling the bottleneck is that the server does not send blocks fast enough to the client (falling is controlled by client). In the case of a (reasonably fast) cart, the server does not load blocks fast enough; this is evident even on fast servers if there are many players. In this case you are right, the client is not the issue, because it is the server that does the movement calculation. But it has to load the blocks in order to do the movement calculation correctly, otherwise the cart hits the grey and keeps going -- in my experience, up to 100 blocks before the server realizes that the rail changed direction long before then. I suspect the reason for this particular problem is that the server is too busy trying to send blocks to the client (which has gone off into the grey) instead of prioritizing the loading of blocks where the rail actually is. Hope this makes sense, I am unsure how to condense it. I just had a cool idea: I can modify the cart mod on my server to preload the rail ahead of the player. Even if there's no API yet to instruct the server to send specific blocks to the client, in this case it won't matter, as long as the rail is loaded server-side. Fun! |
OK... What I tried was this: BUT... It turns out that when a player sits in a cart (or a boat) the speed is not reported correctly from the client. Seems the client says the player's speed is 0, since the cart is moving, not the player. AND... Estimating the actual speed at the server is unreliable, even when I put some dampening on the calculation. |
@proller your comment got lost in the discussion. I looked the code you pointed out, but I do not quite get it. Any chance you can explain? |
I guess you can get the velocity of the object the player is attached to. |
Proller's code:
Looks like it does similar to your other PR but in a more hacky way. For large player speed and for d <= 2 it loads in a narrow cylinder of blocks in the speed direction. |
@HybridDog how reliable is this? Who keeps track of the speed in this case? It looks like the speed reported by the client is more of a "desired speed", i.e. the speed requested by the user, not the actual speed. What I have here will always be correct since the server calculates the speed based on the actual succession of positions. |
The velocity, position and acceleration of objects other than the player are defined server side. The client only extrapolates the velocity and position to avoid stuttering. |
I'm going to revive this, now that my other PRs in this area are in. |
I'd like to revive this. The mistake I made earlier was to not include dtime in the calculation. |
You could also revive this PR and apply your new changes to the already existing branch. However, if you prefer starting with an empty discussion again, it's also possible to open a 2nd PR (also for the same branch name IIRC). |
Very simple and stupid player position prediction.
When a player is not (or slowly) moving, the logic is unchanged.
But if a player is moving fast (f.e. flying in fast mode) the server will prefer to send block some distance out in the direction the player is traveling.
This effect is capped at 50% of the visible range - that is farthest shift the player will allow.
This makes for a smoother experience. Human players tend to look into the direction they move, and then part will be rendered sooner. Capping it at 50% of the view range avoids rendering faraway "block-islands".
Note: Part of this effect existed already, but it was set to exactly one MAPBLOCK out in the player's moving direction regardless of the player's speed. This makes it speed dependent.