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.

Sign up for GitHub

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

Jump to bottom

Fix computation of view range (in blocks) sent to server #4882

Merged
merged 1 commit into from
Dec 11, 2016
Merged

Fix computation of view range (in blocks) sent to server #4882

merged 1 commit into from
Dec 11, 2016

Conversation

Rogier-5
Copy link
Contributor

Fixes #4878

@est31 est31 added the Bugfix 🐛 PRs that fix a bug label Dec 10, 2016
@est31 est31 added this to the 0.4.15 milestone Dec 10, 2016
@lhofhansl
Copy link
Contributor

Can you elaborate? :)

With wanted_range = 20, you'd get 19/17 now and 20/16 before, both would end up being 1.

Does that fix the problem for you?

I think there is another problem with the server only sending with 1 map block send distance. I had thought the fix is to send at least with a map block radius of 2.

@juhdanad
Copy link
Contributor

Yes, the bug is fixed.
@lhofhansl it is 19/16+1 now, which is 2.

@Rogier-5
Copy link
Contributor Author

Rogier-5 commented Dec 10, 2016 •
edited
Loading

@est31 It does fix the issue I am seeing, and which I think is the bug @juhdanad reported.
(Edit: that should have been @lhofhansl)

With wanted_range_nodes = 20:
Previously: wanted_range_blocks = 20/16 = 1
With this fix: wanted_range_blocks = 19/16 + 1 = 1 + 1 = 2

Basically, I changed the formula from:
wanted_range_blocks = floor(wanted_range_nodes / 16.0)
to
wanted_range_blocks = ceil(wanted_range_nodes / 16.0)

Maybe I should add some more clarification...

sfan5
sfan5 reviewed Dec 10, 2016
View reviewed changes
src/client.cpp
@@ -939,7 +939,7 @@ void writePlayerPos(LocalPlayer *myplayer, ClientMap *clientMap, NetworkPacket *
u32 keyPressed = myplayer->keyPressed;
// scaled by 80, so that pi can fit into a u8
u8 fov = clientMap->getCameraFov() * 80;
u8 wanted_range = clientMap->getControl().wanted_range / MAP_BLOCKSIZE;
u8 wanted_range = (clientMap->getControl().wanted_range - 1) / MAP_BLOCKSIZE + 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we have a ceil() function for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is double ceil(double). But then there'd be a useless conversion to floating point and back. Whereas integer arithmetic does the job. I am not aware of any int ceilDiv(int, int) or so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that wanted_range is in fact already a float. I assumed it was integral...

Changed to ceil().

@lhofhansl
Copy link
Contributor

Oh. I see. Of course. 👍

@lhofhansl
Copy link
Contributor

lhofhansl commented Dec 10, 2016 •
edited
Loading

Now we can also change this, perhaps to 0.95. Or get rid of the factor altogether.

@Rogier-5
Copy link
Contributor Author

Rogier-5 commented Dec 10, 2016 •
edited
Loading

Now we can also change this, perhaps to 0.95.

Won't 1.0 work ?

IIRC the fog formula reaches 100% fog before it reaches the max viewing range. So there should be no need to artificially reduce the viewing range.

Edit: tested 1.0, and the results seem good with viewing ranges of 20 and 30. In both cases, nodes that are at viewing_range - 2 are not distinguishable.

@lhofhansl
Copy link
Contributor

lhofhansl commented Dec 10, 2016 •
edited
Loading

Yeah. 1.0 works. I was wondering why that was still needed. You found the answer.
I just tried to simply remove it (i.e. 1.0) and it looks fine in all cases. Wanna just do that as part of this PR?

@paramat would like that too :)

@lhofhansl
Copy link
Contributor

Cool. This all is coming together nicely. This is a great community.

@Rogier-5
Copy link
Contributor Author

Rogier-5 commented Dec 10, 2016 •
edited
Loading

Wanna just do that as part of this PR?

Well... I've already got the code. Just need to commit and push...

Edit: pushed.

@lhofhansl
Copy link
Contributor

lhofhansl commented Dec 10, 2016 •
edited
Loading

@Rogier-5 I meant removing the 0.9 fog range factor as part of this PR.

@lhofhansl
Copy link
Contributor

Ah. You did already. Cool.

@Rogier-5 Rogier-5 mentioned this pull request Dec 10, 2016
Closed
@Rogier-5
Fix computation of viewing range (in blocks) sent to server
6c2c75c 
Fixes minetest#4878

Also remove an artificial viewing range reduction that
(presumably) was added to compensate for miscomputed
viewing ranges, and that doesn't seem to be needed any
more (thanks to lhofhansl).
paramat
paramat reviewed Dec 11, 2016
View reviewed changes
src/game.cpp
@@ -4214,7 +4214,7 @@ void Game::updateFrame(ProfilerGraph *graph, RunStats *stats,
if (draw_control->range_all) {
runData->fog_range = 100000 * BS;
} else {
runData->fog_range = 0.9 * draw_control->wanted_range * BS;
runData->fog_range = draw_control->wanted_range * BS;
Copy link
Contributor

@paramat paramat Dec 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure about this, the 'wanted range' here is surely a different local value in the client? the 'ceil(wanted range)' is only sent to the server?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a high chance i'm wrong though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops i misunderstood, this is fine.

@lhofhansl
Copy link
Contributor

Tested this. I have not been able to produce any visible edges at any view range.

We discussed this on #4811, there I said I can see edges when this removed. Turns out that was due to me sending inadvertently floor of view-range / MAP_BLOCKSIZE. Due to that we'd retrieve sightly less on terms of range. I remember I was wondering back then; now I have an explanation. :)
Pretty certain this is fine.

@paramat
Copy link
Contributor

paramat commented Dec 11, 2016

Will test.

@paramat
Copy link
Contributor

paramat commented Dec 11, 2016

Works 👍

@lhofhansl
Copy link
Contributor

We now have this and #4884, and some of the logic is duplicated.
Let's get this one in, and then do #4884 on top. (that'd be my personal preference at least)

@est31
Copy link
Contributor

est31 commented Dec 11, 2016

👍

@est31 est31 merged commit 6077207 into minetest:master Dec 11, 2016
@Rogier-5 Rogier-5 deleted the view-range branch December 11, 2016 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfan5 sfan5 sfan5 left review comments

@paramat paramat paramat left review comments

Assignees
No one assigned
Labels
Bugfix 🐛 PRs that fix a bug High priority One approval ✅ ◻️
Projects
None yet
Milestone
0.4.15
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Rogier-5 @lhofhansl @juhdanad @paramat @est31 @sfan5