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

42Send only changed node metadata to clients instead of whole mapblock #3848

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
10 participants
@est31
Copy link
Contributor

commented Mar 12, 2016

Rebased version of #3775 (and #3166).

Also added some of my own improvements. Will squash before merge.

node_meta_updates.remove(event->p);
node_meta_updates.push_back(event->p);

if (MapBlock *block = m_env->getMap().getBlockNoCreateNoEx(

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Mar 12, 2016

Member

Could you split this into an assignment and a check?

This comment has been minimized.

Copy link
@est31

est31 Mar 12, 2016

Author Contributor

loool... it was that way before nrz demanded it to be this way.

i != clients.end(); ++i) {
meta_updates_list->clear();
Player *player = m_env->getPlayer(*i);
RemoteClient* client = m_clients.lockedGetClientNoEx(*i);

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Mar 12, 2016

Member

Star in the wrong place :P

for (std::vector<u16>::iterator i = clients.begin();
i != clients.end(); ++i) {
meta_updates_list->clear();
Player *player = m_env->getPlayer(*i);

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Mar 12, 2016

Member

Under what circumstances would player be null and client be non null?

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Mar 12, 2016

Member

When the client is not emerged yet? If they're not emerged, will they have blocks stored that need modifying?

This comment has been minimized.

Copy link
@est31

est31 Mar 12, 2016

Author Contributor

I have no idea. In the past I had added an errorstream print to the else part of one of these checks, and it complained to me from time to time.

Removing the check here as you propose would possibly lead to the introduction of a hard to reproduce bug.

This comment has been minimized.

Copy link
@nerzhul

nerzhul Mar 19, 2016

Member

exactly. Player object is kept in memory, not client.

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2016

Yes, its only about details.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2016

Failed to patch with the diff, networkprotocol.h hunk failed at 135.

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2016

hrmm might have to rebase...

RealBadAngel and others added some commits Feb 23, 2016

@est31 est31 force-pushed the est31:meta_stuff branch to c0c4dd8 Mar 14, 2016

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2016

Rebased.

if (!meta) {
continue;
}
if (player) {

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Mar 14, 2016

Member

I was asking due to the if here - maybe the player check should be done before the inner loop. I don't know.

This comment has been minimized.

Copy link
@est31

est31 Mar 21, 2016

Author Contributor

It needs to be inside the inner loop unfortunately as it depends on pos. The only thing that can be put outside is the v3f player_pos calculation, but thats not worth it IMO.

@ghost

This comment has been minimized.

Copy link

commented Mar 14, 2016

It seems it's very fast. Inventory to and from locked chests feels more responsive.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

Tried it with vending machines, no more draw time spikes 👍

NodeMetadataList *meta_updates_list = new NodeMetadataList();
std::string datastring = pkt->readLongString();
std::istringstream is(datastring, std::ios::binary);
std::ostringstream oss(std::ios::binary);

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Mar 20, 2016

Member

Just use stringstream so you don't need all these copies.

@paramat

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

How's this going? This really should be in release, i'll delay release if necessary.

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2016

hmmm its spamming me with WARNING[Main]: Map::setNodeMetadata(): Block not found messages, no idea where they came from...

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2016

Are you sure it is with 3848 only, or maybe it is spamming in default minetest? I've noticed them too but have not seen any effects.

@kilbith

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2016

I noticed this message too, without this patch of course.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2016

I think I have server crash with this. Look #3897 , sorry for posting it in issues, was not sure if 3848 was responsible.

@paramat

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

its spamming me with WARNING[Main]: Map::setNodeMetadata(): Block not found messages

Sometimes i get that in master every few seconds.

@paramat paramat removed this from the 0.4.14 milestone Mar 29, 2016

@paramat paramat removed the High priority label Mar 29, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

Due to limited dev time, and because this can introduce subtle bugs, we have decided to work on this for longer and not rush it for this release.

@paramat

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

From IRC, not possible for this release because:

hmmmm > no that's not going to be possible due to the protocol change

p16 &= MAP_BLOCKSIZE * MAP_BLOCKSIZE - 1;
p.Y = p16 / MAP_BLOCKSIZE;
p16 &= MAP_BLOCKSIZE - 1;
p.X = p16;

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Jul 18, 2016

Member

How about this one? Two divisions and no multiplication: (minimal improvement)

p.X = p16 & (MAP_BLOCKSIZE - 1);
p16 /= MAP_BLOCKSIZE;
p.Y = p16 & (MAP_BLOCKSIZE - 1);
p16 /= MAP_BLOCKSIZE;
p.Z = p16;

This comment has been minimized.

Copy link
@sfan5

sfan5 Oct 9, 2016

Member

I suggest not touching this code.

@paramat paramat added this to the 0.4.15 milestone Oct 9, 2016

@est31

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2016

Sorry I don't have time right now to work on this. I'd prefer if someone else took over.

writeU16(os, p.X);
writeU16(os, p.Y);
writeU16(os, p.Z);
} else {

This comment has been minimized.

Copy link
@sfan5

sfan5 Oct 9, 2016

Member

What is this needed for?

@paramat paramat removed the Enhancement label Oct 29, 2016

@paramat paramat removed this from the 0.4.15 milestone Dec 7, 2016

@sfan5 sfan5 force-pushed the minetest:master branch to 09f1a0c Dec 21, 2016

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2017

Who wants some? This was in development since RBA times, it needs to be finished to improve performance in technic and mesecons areas (should increase fps).

@paramat

This comment has been minimized.

Copy link
Member

commented Jan 30, 2017

Adorable PR needs a loving home.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

Continued in #5268.

@SmallJoker SmallJoker closed this Mar 18, 2017

@celeron55 celeron55 changed the title Send only changed node metadata to clients instead of whole mapblock 42Send only changed node metadata to clients instead of whole mapblock Mar 18, 2017

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.