Fix incorrect distance computation for visible blocks #4765
Conversation
Rogier-5
force-pushed
the
mapblock-render-distance
branch
from
cc27444
to
295ec77
Compare
|
What if you would increase the range in this function by |
|
👎 This may work perfectly, but too perfectly, it is a lot of code to put into 'isBlockInSight()' which needs to be as lightweight and fast as possible. Better to just make a very simple imperfect fix by changing a single number somewhere, such as the recently used 'range', such that mapblocks are drawn slightly beyond the fog instead of slightly in front. |
|
I also feel that this is bit overkill perhaps. We use the center of the mapblock, which is not perfect, but it seems good enough. Also note that isBlockInSight is used in clientIface.cpp to control the retrieval of blocks from the server. Need to make sure this is not affected. |
|
Yeah better to pass an adjusted value for range elsewhere. |
|
I guess this started happening due to e7c62ed ? |
|
I doubt this has anything to do with e7c62ed. There we do not use the fog distance or anything, but use max_block_send_distance, which is fixed. Also in these cases the block has already been seen by the client, so it wouldn't disappear again. |
|
Ah yes i was mixing things up. |
Rogier-5
force-pushed
the
mapblock-render-distance
branch
from
295ec77
to
5ed8927
Compare
|
Simpler code here may indeed give a better performance tradeoff than more processing later, especially since the graphics processor will be doing part of that extra work. @lhofhansl: The patched code necessarily selects more blocks, so the retrieval of blocks from the server would only be affected in the sense that network traffic and server load might increase, but for a good cause. @SmallJoker's suggestion to adjust with half the maximum block diameter instead sounds good, and it solves the issue. I have adjusted the distance that is computed ( |
|
Thanks @Rogier-5 . What I was trying to say was: Let's adjust the radius at the place where we call isBlockInSight. I do not think we want to increase the radius for block retrieval, so let's not change isBlockInSight, but change the relevant callers. |
|
Actually I take that back. In e7c62ed I pass in (full_d_max + 1) * BS * MAP_BLOCKSIZE to range. With the change here we can do away with the +1 part in there, which would make sightly more sense. Lemme try that. |
The blocks that are rendered will have to be fetched from the server at some point... What is the reason for wanting to fetch less blocks ? If fetching less blocks is the goal, then it seems more logical to implement that at the location where the blocks are fetched from the server. I don't know how that is done ATM, but I suppose far-away blocks could be transferred less often anyway.
Would that be more efficient ? Because IMHO it would certainly be more obscure: adjusting the range everywhere else, instead of in a single place, inside BTW: I tried the patch without adjusting |
|
OK. I changed Interestingly, the returned value of d has to change for your fix to work. For kicks I just changed it to In any case. +1 to this change now. Can you also make the change I mention in clientiface.cpp? (Without that we'd be increasing the retrieval range). |
Rogier-5
force-pushed
the
mapblock-render-distance
branch
from
5ed8927
to
6b7e4a2
Compare
|
Perhaps Instead of the if-statement (but now I'm nitpicking). |
|
^ Yes that would be preferable. This PR looks good now 👍 as it seems you have both tested. |
|
👍 on latest. The MYMAX thingy looks a bit nicer, but that's it. |
The client would not compute the distance from the camera to to a mapblock correctly. The result was that blocks that were in view (i.e. not beyond the fog limit) would not be rendered. With the improved distance computation, a range adjustment that existed in clientiface.cpp is no longer required.
Rogier-5
force-pushed
the
mapblock-render-distance
branch
from
6b7e4a2
to
5d448f2
Compare
|
OK. Changed |
|
seems good |
|
👍 |
| */ | ||
| bool isBlockInSight(v3s16 blockpos_b, v3f camera_pos, v3f camera_dir, | ||
| f32 camera_fov, f32 range, f32 *distance_ptr) | ||
| { | ||
| // Maximum radius of a block. The magic number is | ||
| // sqrt(3.0) / 2.0 in literal form. | ||
| const f32 block_max_radius = 0.866025403784 * MAP_BLOCKSIZE * BS; |
There was a problem hiding this comment.
@Rogier-5 Just for future reference, some of us prefer that stuff like this is static const qualified rather than just const qualified. I doubt it makes a difference with modern compilers (easy to check) and it's more of a "style" issue, but static const is IMO (from code I look at is more "common" and "idiomatic") is what I prefer, and obviously some others in #minetest-dev and other programming channels agree. It's a nitpick; not important really; and just "one of those things", which is why this was merged as-is (plus is consistent with code above anyway). Just thought I'd mention it for interests sake.
|
@Zeno- Thanks. Actually, I did consider static, but I reasoned it wouldn't make a difference anyway (compiler-wise), so I left it out... I'll use it next time. |
The client would not compute the distance from the camera to
to a mapblock correctly. The result was that blocks that were in
view (i.e. not beyond the fog limit) would not be rendered.
See issue #4763 for a discussion and screenshots