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

Send to clients only changed node metadata instad of whole mapblock #3166

Closed
wants to merge 1 commit into from

Conversation

RealBadAngel
Copy link
Contributor

thx to sending single meta packet is very small, circa 200-300 bytes for a chest, and it doesnt cause mapblock mesh updates (not only origin block mesh was updated but also neighbours).
Also sending whole block was causing each meta for that block being deleted and restored.
Effect on VE server, known to be laggy as hell and with usually slidshow (~10 fps as on my box):
screenshot_20150909_191925

@@ -620,6 +622,12 @@ enum ToClientCommand
u32 id
*/

TOCLIENT_NODEMETA_CHANGED = 0x54,
/*
u16 command
Copy link
Contributor

Choose a reason for hiding this comment

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

nrz wants to not document the command field, as it exists in any packet (there is no extra information).

Copy link
Member

Choose a reason for hiding this comment

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

@est31 this is exact, command field is not useful information there

@est31
Copy link
Contributor

est31 commented Sep 10, 2015

You'll have to adjust how the p param of MEET_BLOCK_NODE_METADATA_CHANGED is handled (single position instead of blockpos) everywhere it is used (including rollback_interface.cpp).

@est31
Copy link
Contributor

est31 commented Sep 10, 2015

Most of the PR looks good, except of things I've already pointed out above, but generally we should be very cautious with applying this. @kwolekr can you review this PR?


MapBlock *block = m_env->getMap().getBlockNoCreateNoEx(blockpos);
if(block)
block->raiseModified(MOD_STATE_WRITE_NEEDED);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, we have to take care of storage of the modified block in the map database. We can spare sending it, thats fine, but we shouldn't avoid storing them into the map.

Copy link
Member

Choose a reason for hiding this comment

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

Done in Server::sendMetadataChanged

Copy link
Contributor

Choose a reason for hiding this comment

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

yea

@RealBadAngel
Copy link
Contributor Author

IMHO I dont have to comment using p parameter as single node coords for this event, it means it for each other case already, MEET_BLOCK_NODE_METADATA_CHANGED was an exception to that rule so it was documented.
I have removed raising block as modified from l_nodemeta.cpp and rollback_interface.cpp because its done in the event handler, no need to mark the block three times.
As for the command in packet description: all others have command mentioned in it, why i should format it other way?

@est31
Copy link
Contributor

est31 commented Sep 10, 2015

Okay, i see you call raiseModified, sorry. And I agree now that it doesn't has to be documented that p indicates the position, I had removed my comment.

@est31
Copy link
Contributor

est31 commented Sep 10, 2015

As for the command in packet description: all others have command mentioned in it, why i should format it other way?

Not all others, all new and modified packets have the command not mentioned.

@est31
Copy link
Contributor

est31 commented Sep 10, 2015

As for the command in packet description: all others have command mentioned in it, why i should format it other way?

Now situation changed: 64a5eec

@RealBadAngel
Copy link
Contributor Author

Ive removed "command" from packet desc

@VanessaE
Copy link
Contributor

Provides a small improvement for me - a few fps better than before anyway. Motion seems smoother, less jerky.

std::istringstream iss(oss.str(), std::ios::binary);
meta->deSerialize(iss);
try {
m_env.getMap().setNodeMetadata(p, meta);
Copy link
Member

Choose a reason for hiding this comment

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

if this return false you MUST delete meta pointer

@RealBadAngel
Copy link
Contributor Author

@VanessaE and for info to all others: she really has problem with drivers, with her decent HW setup she can barely hit 40 fps....
She seems to be capped by the driver.
I really started from 5-10fps on her server and now im getting 50-60

@cheapie
Copy link
Contributor

cheapie commented Sep 13, 2015

@RealBadAngel Then why do her and I both get way better frame rates with other games, even ones with much fancier graphics?

@est31
Copy link
Contributor

est31 commented Sep 17, 2015

Just wanting to point out that the bug with this PR that you get all metadata changes on the whole server, not just the ones you are close to still remains unfixed.

@SmallJoker
Copy link
Member

@Fixer-007 It required some manual changes to merge it with upstream.
However, here's your Win32 binary, *.7z archive
This client is useless on servers which run on another/an older version.

@Fixer-007
Copy link
Contributor

Thank you very much!

@RealBadAngel
Copy link
Contributor Author

continued in #3775

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.

None yet

8 participants