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

Support entity properties > 1 MTU #830

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HifiExperiments
Copy link
Member

I am not familiar with this part of the code at all. This is a highly experimental and possibly dangerous change. But...it seems to work (in my basic tests)!

There are two parts to this change:

Client -> Server Communication (EntityEditLarge)

Clients communicate edits via EntityEditPacketSender::queueEditEntityMessage, which calls EntityItemProperties::encodeEntityEditPacket. This gets called multiple times, packing all the changed properties into multiple individual packets. If the first property in a new packet doesn't fit (reported via firstDidntFitProperty), we know it's too big, and we instead search for a large enough buffer and send an EntityEditLarge packet list.

Server -> Client Communication (EntityDataLarge)

The server is a little more complicated. It is constantly trying to send multiple entities to multiple clients as things change. OctreeSendThread::traverseTreeAndSendContents builds up _packetData in EntityTreeSendThread::traverseTreeAndBuildNextPacketPayload and sends it as EntityData packets. Now, if the first property we encounter doesn't fit (_numEntities == 0 && firstDidntFitProperty was set), we again search for a large enough buffer and send a EntityData packet list...a little circuitiously through handlePacketListSend.

Notes:

  • EntityDataLarge packets are compressed later in the process, which means we can detect that we need a packet list based on the uncompressed size, but then end up fitting everything in one packet after compression. We don't currently do anything special to handle this, so some of our large packets can actually end up quite small. EntityEditLarge packets are not compressed (maybe they should be?).
  • This isn't a protocol change! I used some unused packet types in PacketHeaders.h, so theoretically old clients/servers will simply ignore these new packet types. Alternatively, this could theoretically be done with the same old packet types and a protocol change, but it seems helpful to be able to keep track of these new packets separately.
  • This specifically handles packets to and from the entity server. It does not handle avatar entity packets (which go through the avatar server) or any other packets that break when they get too big (avatar joints).

@HifiExperiments HifiExperiments added enhancement New feature or request help wanted Extra attention is needed needs CR This pull request needs to be code reviewed needs QA This pull request needs to be tested server If used for a Pull Request, server packages are going to be built. labels Feb 23, 2024
EntityPropertyList targetProperty = firstDidntFitProperty;
unsigned int bufferMultiplier = 2;
int32_t numEntitiesOffset;
while (true) {
Copy link
Member Author

Choose a reason for hiding this comment

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

appendEntityData still doesn't report how much it overran the buffer by, so we have to do this loop to search for the right size. this seems dangerous (since there's no upper limit) and slow (since we could know the exact right size after the first attempt). we should definitely fix the latter issue in a followup, but I'm not sure how bad the lack of an upper limit really is.

@@ -119,15 +121,29 @@ void EntityEditPacketSender::queueEditEntityMessage(PacketType type,
requestedProperties -= PROP_PRIVATE_USER_DATA;
}

while (encodeResult == OctreeElement::PARTIAL) {
encodeResult = EntityItemProperties::encodeEntityEditPacket(type, entityItemID, propertiesCopy, bufferOut, requestedProperties, didntFitProperties);
while (encodeResult == OctreeElement::PARTIAL && !requestedProperties.isEmpty()) {
Copy link
Member Author

@HifiExperiments HifiExperiments Feb 23, 2024

Choose a reason for hiding this comment

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

weirdly, entity adds from client -> server already used packet lists, but have an arbitrary 10 * MTU limit (see MAX_ADD_DATA_SIZE above). so Entities.addEntity with an extremely large property (or several big properties) could still fail.

NLPacketList::NLPacketList(const NLPacketList& other) : PacketList(other) {
// Update _packets
for (auto& packet : _packets) {
packet = NLPacket::fromBase(std::move(packet));
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a copy constructor so this move is wrong, but I couldn't get it to work otherwise...(because of the const if I remember correctly)

@ksuprynowicz
Copy link
Member

I just tested the PR, and when starting game in tutorial world it segfaults in libraries/entities/src/EntityItem.cpp line 391.
Here's the backtrace and local variables:
image

@ksuprynowicz
Copy link
Member

It's possible that this affects serverless worlds only

@ksuprynowicz
Copy link
Member

The crash seems to happen every time for me when launching interface in Tutorial.

@gydence
Copy link

gydence commented Mar 1, 2024

thank you, I’ll take a look! looks like it’s specific to when you have an avatar entity attached

@HifiExperiments
Copy link
Member Author

@ksuprynowicz should be fixed!

@ksuprynowicz
Copy link
Member

The crash is fixed now :)
It seems that there's another problem though. I started a server and client on this version, and large part of the entities on that copy of Overte Hub were missing. I will do more teating tomorrow.

@HifiExperiments
Copy link
Member Author

@ksuprynowicz interesting - are there even any entities in the domain that hit the packet limit? if there are none I'm surprised, as the code should operate as before.

if there are any, I did encounter a similar issue at one point during testing, but I thought I had fixed it. it related to the sequence number and the changes in OctreeSendThread.cpp. when we send a large packet, we rewrite the sequence number of the normal size packet that we have at the time to bump it. but now that we're sending a mix of normal packets and packet lists, I don't totally understand if the sequence numbers are able to handle it.

@ksuprynowicz
Copy link
Member

Here's a screenshot of the same place in master branch and in PR 830:
Master branch:
overte-snap-by--on-2024-03-10_17-41-31

PR 830:
overte-snap-by--on-2024-03-10_18-52-59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed needs CR This pull request needs to be code reviewed needs QA This pull request needs to be tested server If used for a Pull Request, server packages are going to be built.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants