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

Keep mapblocks in memory if they're in range #10714

Merged
merged 13 commits into from
Feb 26, 2021

Conversation

hecktest
Copy link
Contributor

@hecktest hecktest commented Dec 9, 2020

Fixes #10567

Rendering, bugfix
Ready for review

With this PR, the client no longer discards mapblocks based on the result of a camera frustum cull. This should reduce the appearance of holes in a map, and prevent the client from hammering the server for the same blocks over and over.
It also makes frequent client data unloads a viable performance strategy, which may help #10683
The range used for unloading is padded a bit beyond the visual cull range.

Some other minor parts of clientmap.cpp have been cleaned up along the way:

  • A couple lines have been hoisted out of a loop that they weren't dependent on
  • A duplicate frustum cull that only effectively computed distance for block animation was replaced by an actual distance calculation.

src/client/clientmap.cpp Outdated Show resolved Hide resolved
src/client/clientmap.cpp Outdated Show resolved Hide resolved
src/client/clientmap.cpp Outdated Show resolved Hide resolved
src/client/clientmap.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 added Bugfix 🐛 PRs that fix a bug Performance labels Dec 14, 2020
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works.

@KaylebJay
Copy link

Would be good to get this into 5.4 & improve a lot of client's experiences!

@sfan5
Copy link
Member

sfan5 commented Feb 24, 2021

I remember that there was some disagreement about whether this should be done (where?), but I'm inclined to say we can just merge this and see if any issues crop up.

Or maybe we should preemptively add a setting so this behaviour can be disabled on Android (to save RAM). Thoughts?

@celeron55
Copy link
Member

I'll say merge it. I don't even remember anymore why it's not like this already.

@sfan5 sfan5 merged commit 225e690 into minetest:master Feb 26, 2021
@lhofhansl
Copy link
Contributor

The reason I was questioning this was that you had to avert your eyes from a set of blocks for 10 minutes (default) until they get unloaded. Now we keep invisible blocks around even when you haven't looked at them for 10 minutes. Not a big deal.

Before 738f624 this was definitely a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client may unload blocks it shouldn't
6 participants