Fix mob deserialization errors in the client #4743

Rogier-5 · 2016-11-06T16:11:18Z

The problem was seen while using the mobf mod package.

The problem happened when the server would serialize entity attachments.
Sometimes, such attachments no longer exist. The serialization code would
skip those. However, the total number of attachments was serialized
earlier. Therefore the client would expect more than it got, and log a
serialization error.

Foghrye4 · 2016-11-06T16:22:54Z

@Rogier-5 Could you find out, why dead infant still presented in that list? Obviously, its an another bug. Whenever infant dies, he should remove itself from parents set, isn't it?
At my humble opinion it would be more healthier to check if all infants is alive before setting a number of messages and remove dead ones.
Even more healthier will be find out why infants in a moment of they own death not willing to remove themselves from parents set.

Rogier-5 · 2016-11-06T16:34:40Z

Could you find out, why dead infant still presented in that list? Obviously, its an another bug. Whenever infant dies, he should remove itself from parents set, isn't it?

I suppose so. OTOH, often a child should not die to begin with, until the parent does... But that's probably a more complex thing to fix.

Anyway, at this moment, the serialization code and the deserialization code do not match, which causes the deserialization errors. That is a problem, independently of what triggers it. So I agree that the trigger is most probably a bug too, and should be fixed, but that's another PR.

Foghrye4 · 2016-11-06T16:42:44Z

@Rogier-5 May I ask you? I know that feeling when you want a fix and want it fast.
I digged a code a little bit.

Here is a code where "removed" objects removed:
https://github.com/minetest/minetest/blob/9d25242c5c14a11d692254cf910345d51c9a24fa3/src/environment.cpp#L1821

As you can see here, there is no sign of infants removing themselves from parents set.
I suggest you to try to add such code inside removingFromEnvironment() function and see, if this will fix that bug.

EDIT: Wait. What if player attached to entity and log off and then log in back?
Ah, nevermind. It wouldn't be attached back anyway.

nerzhul · 2016-11-06T17:13:46Z

i think the problem is the message count sent by server, not the datas you fixed by adding empty message

Rogier-5 · 2016-11-06T18:11:00Z

i think the problem is the message count sent by server, not the datas you fixed by adding empty message

@nerzhul: indeed. However, to fix the message count, there are basically two options:

iterate the attachment list twice: once to count the number of attachments that actually exist, once to serialize them.
Serialize the bones and attachments to a separate string, and then append that string to the message after writing the message count.

Both require additonal work on every serialization, while the problem itself is relatively rare, and the additional 4 bytes are negligible So I chose this option as the more efficient one.

Rogier-5 · 2016-11-06T19:28:16Z

@Foghrye4: The non-existent-child problem has other associated questions that should be answered: why does the attached entity disappear while the parent still exists ? Was that intended ? Should attached entities stay with the parent in general, or only in specific cases ? Is it necessary to take special measures to avoid either the child or the parent disappearing while the other does not ? What happens when the child is in a block that is being deactivated while the parent is not ? And possibly more.

Foghrye4 · 2016-11-06T19:34:34Z

@Rogier-5

why does the attached entity disappear while the parent still exists ?

Why not? For example player could be attached to cart. Whenever he log-off, cart still remain on place.

Was that intended ?

Indeed. Player will be sad, if he wouldn't find this cart.

Should attached entities stay with the parent in general, or only in specific cases ?

I don't think its a good idea to reattach players on re-login automatically. We should leave that to modders.

Is it necessary to take special measures to avoid either the child or the parent disappearing while the other does not ?

No, by a reasons above.

What happens when the child is in a block that is being deactivated while the parent is not ?

Could not be happened. Child real coordinates are always same as parents.

I tried to spawn two entities, attach to each other, kill one and move around a little. Cant repeat that bug. In what condition that bug happened?

Rogier-5 · 2016-11-06T19:45:28Z

@Foghrye4: child objects are not just players on carts. Mobf uses them for health bars, and I've seen them used for other purposes as well. And I'm sure there are other uses as well that I can't imagine at this moment. Life bars for instance, should remain attached to the parent at all times...

Why not? For example player could be attached to cart. Whenever he log-off, cart still remain on place

And if it's a health bar ? Why does it disappear while the parent is still there ? Or why does the parent disappear while the health bar remains ?

sfan5 · 2016-11-06T20:08:43Z

👍 Fixes a legitimate oversight in serialization code.

Rogier-5 · 2016-11-07T08:10:13Z

@nerzhul: I changed the code to fix the message count if possible, by rewinding the stream and overwriting the expected count with the actual count if they differ.

nerzhul · 2016-11-07T08:48:33Z

is this me or LuaEntitySAO and PlayerSAO serialization functions are very similar. @Rogier-5 do you think it's possible to factorize those functions ? We have a common class for this if needed, UnitSAO

Rogier-5 · 2016-11-07T09:33:39Z

@nerzhul: they are almost identical indeed. Like some other PlayerSAO / LuaEntitySAO methods too... Too much copy/paste coding I suspect :-)

What is the function of class UnitSAO BTW ? It is quite small. Can its functionality be folded into ServerActiveObject ? Or does it serve a purpose that does not allow this ?

I suppose PlayerSAO & LuaEntitySAO could benefit from some refactoring. I'm willing to have a look at it, but it is a more complex operation, and it will probably require that some things be implemented differently (e.g. splitting methods, adding virtual functions, etc.) so that the code can be effectively shared. So I would propose not to delay this small/trivial fix just because the code it fixes could also be refactored.

nerzhul · 2016-11-07T09:37:53Z

UnitSAO was very recent and used when refactoring PlayerSAO model to port some SAO values common between LuaEntitySAO & PlayerSAO.
we cannot use the generic SAO because it could be used at a moment for others SAO which doesn't need unit specific attributes.

For this pr especially, if you have a nice way to factorize the client init data it's good, for other refactor, we will look at this later, not here :)

Rogier-5 · 2016-11-07T14:02:13Z

@nerzhul: I refactored the code, but one change required another, and quite a few things ended up being moved to UnitSAO...

nerzhul · 2016-11-07T14:05:25Z

Very nice refactor, can you move the one line functions to class def instead of the cpp ?

nerzhul · 2016-11-07T14:07:15Z

src/content_sao.cpp

+}
+
+
+std::string UnitSAO::getPropertyPacket()


Is this possible to change this method call to void getPropertyPacket(std::string &result) ? This should avoid a useless copy

The copy operation in getPropertyPacket should be optimized away by the compiler (return value optimization). Also, C++11 supports move semantics, and that should guarantee that the string is not copied while being returned.

okay then, no problem. as you know we are not C++11 aware ATM

nerzhul · 2016-11-07T14:07:33Z

src/content_sao.cpp

+}
+
+
+ItemGroupList UnitSAO::getArmorGroups()


This function should be const

nerzhul · 2016-11-07T14:08:03Z

src/content_sao.cpp

+
+ItemGroupList UnitSAO::getArmorGroups()
+{
+ return m_armor_groups;


Do we modify this result in upper functions ? else could be nice to return const ItemGroupList &

nerzhul · 2016-11-07T14:10:48Z

src/content_sao.cpp

+}
+
+
+// Applies to LuaEntitySAO for protocol >= 14


@est31 @sfan5 @sofar @Zeno- what about dropping conditions between protocol v15 and upper for clientInitializationmessages ? v14 is very very old and i don't think there is v0.3 clients on current servers

nerzhul · 2016-11-07T14:11:44Z

src/content_sao.h

+ void getAttachment(int *parent_id, std::string *bone, v3f *position, v3f *rotation);
+ void addAttachmentChild(int child_id);
+ void removeAttachmentChild(int child_id);
+ UNORDERED_SET<int> getAttachmentChildIds();


if we didn't write on the result, could be very good to use const ref on the result

nerzhul · 2016-11-07T17:48:09Z

src/content_sao.cpp

+
+bool UnitSAO::isAttached()
+{
+ if(!m_attachment_parent_id)


missing space

nerzhul · 2016-11-07T17:48:27Z

src/content_sao.cpp

+ if(!m_attachment_parent_id)
+ return false;
+ // Check if the parent still exists
+ ServerActiveObject *obj = m_env->getActiveObject(m_attachment_parent_id);


return m_env->getActiveObject(m_attachment_parent_id) != NULL;

nerzhul · 2016-11-07T17:48:37Z

src/content_sao.cpp

+void UnitSAO::sendMandatoryDataToClient()
+{
+ if(!m_properties_sent)
+ {


space + brace

nerzhul · 2016-11-07T17:48:56Z

src/content_sao.cpp

- os<<serializeString(""); // name
- writeU8(os, 0); // is_player
+ writeU8(os, protocol_version < 14 ? 0 : 1); // version
+ os<<serializeString(""); // name


nerzhul · 2016-11-07T17:50:17Z

src/content_sao.h

+ void notifyObjectPropertiesModified() { m_properties_sent = false; }
+ void setAnimation(v2f frame_range, float frame_speed, float frame_blend, bool frame_loop);
+ void getAnimation(v2f *frame_range, float *frame_speed, float *frame_blend, bool *frame_loop) const;
+ void setBonePosition(const std::string &bone, v3f position, v3f rotation)


const v3f &

I'll change it, but I doubt copying 4 fewer bytes (on a 64-bit platform) will have any measurable performance impact.

Rogier-5 · 2016-11-09T14:13:11Z

OK. I reviewed the code I moved for coding style issues.

As I made so many changes already, I could as well make some more. However, as they change existing code without a real need, I'm not sure they are desired. So they are in a separate commit:

I moved small LuaEntitySAO and PlayerSAO functions to content_sao.h as well
I made some more functions from LuaEntitySAO and PlayerSAO const (with one or two consequences in ServerActiveObject, ActiveObject, ClientActiveObject, etc.).

I'll squash the two commits if the second commit is acceptable.

nerzhul · 2016-11-09T15:09:32Z

src/content_sao.h

@@ -26,6 +26,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "constants.h"
 #include "genericobject.h"
 #include "environment.h"
+#include "remoteplayer.h"


Why include this in the header ?

Is this not possible to add just

class RemotePlayer;

and include the header in the CPP file ?

m_player is dereferenced in PlayerSAO::getDescription...

nerzhul · 2016-11-09T15:10:47Z

Very nice commit for the constness, just one little thing to improve the compilation speed a little bit

@sfan5 @Zeno- @sofar can you review too, this is a very nice fix + refactor

nerzhul · 2016-11-09T15:16:06Z

Exact then the last change is okay for me, i need to re-do a global review

sfan5

This is supposed to fix a bug, not refactor stuff. Please open a seperate PR for the refactor

nerzhul · 2016-11-09T18:34:06Z

@Rogier-5 did you thinks it's possible to have two commits on this good feature ?

Rogier-5 · 2016-11-09T19:11:14Z

Moved the refactorization to a separate PR. See #4760

sfan5

I dislike seeking backwards in the stream, isn't it possible to count in advance?

Rogier-5 · 2016-11-09T20:03:48Z

@sfan5: I don't know if it makes a difference, but the seeking backwards is in the std::ostringstream that is local to the function itself. Basically, the code is writing in another location in a local buffer.

As the number of messages is not known in advance, there are basically three ways to avoid seeking:

Count the number of messages in advance. This duplicates work, as it will be necessary to iterate through the attached entities container one additional time to do the counting.

In addition, it will be less maintainable after the refactor, as the counting will require a separate base class method, and adding a new message to be serialized will mean that both the serialization method and the counting method will need to be changed in the same way.
Serialize the messages to a separate string, and append that string to the string containing the first part of the data after serializing the count. This means data will be copied around an additional time, just to avoid the seeking (which is an O(1) operation, whereas the copying is O(n)).
Insert dummy, empty messages into the list just to make up for the 'missing' messages.

After refactoring the common serialization code into the base class, this will also require a separate counting method in the base class, because, in the derived class, no assumptions can be made about the number of messages that the base class method will serialize. Even though, in this case, the counting method will be allowed to over-estimate the number of messages, adding one is still a maintenance liability (see above).

With string copying being the only practical alternative, I judged that seeking was preferable to doing the additional string copy.

est31 · 2016-11-10T01:08:38Z

I agree with @sfan5, seeking is bad. I'd prefer counting, like:

int message_count = 6 + m_bone_position.size();
for (UNORDERED_SET<int>::const_iterator ii = m_attachment_child_ids.begin();
        ii != m_attachment_child_ids.end(); ++ii) {
    if (ServerActiveObject *obj = m_env->getActiveObject(*ii))
        message_count++;
    }
}

Plus I don't like the messages++ everywhere. I agree that maintainability is important, you can add a comment for that though.

It requires no additional function on the base class or similar, or am I wrong?

The problem was seen while using the mobf mod package. The problem happens when the server serializes entity attachments. Sometimes, such attachments no longer exist. The serialization code skips those. However, the total number of attachments was serialized earlier. Therefore the client expects more than it gets, and logs a serialization error.

Rogier-5 · 2016-11-10T11:23:16Z

@est31: It is not known in advance how many objects are actually attached: some attachments in the list may not exist. So either it will have to estimated, and then the serialized data may have to contain a number of empty messages, or it will have to be counted in advance, requiring extra container iterating.

With the refactor, part of the code (the part which is shared between LuaEntitySAO and PlayerSAO) will move to the common base class. The only maintainable way to obtain the number of messages that the base class will serialize is to call a method in the base class. Adding a magic number in a derived class representing the number of messages the base class will serialize is not an option of course.

So, the simplest and most maintainable solution IMO is to serialize the messages to a string, and append that string to the rest of the serialized data after serializing the count.

I pushed a new version that does that, and I also eliiminated most of the counting.

nerzhul · 2016-11-10T13:34:17Z

Nice, great job @Rogier-5

Foghrye4 mentioned this pull request Nov 6, 2016

Fix crash when attached object no longer exists #4742

Merged

sfan5 added @ Server / Client / Env. Bugfix 🐛 PRs that fix a bug labels Nov 6, 2016

sfan5 added One approval ✅ ◻️ Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines labels Nov 6, 2016

Rogier-5 force-pushed the mob-deserialization branch from 5005fcc to 75bcee3 Compare November 7, 2016 07:50

Rogier-5 force-pushed the mob-deserialization branch from 75bcee3 to e74b406 Compare November 7, 2016 13:39

nerzhul reviewed Nov 7, 2016

View reviewed changes

src/content_sao.cpp

}

ItemGroupList UnitSAO::getArmorGroups()

Copy link

Member

nerzhul Nov 7, 2016

Choose a reason for hiding this comment

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

This function should be const

nerzhul reviewed Nov 7, 2016

View reviewed changes

nerzhul removed the Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines label Nov 7, 2016

nerzhul changed the title ~~Fix mob deserialization errors in the client~~ Fix mob deserialization errors in the client + code factorization Nov 7, 2016

Rogier-5 force-pushed the mob-deserialization branch from e74b406 to 9161d95 Compare November 7, 2016 16:50

nerzhul requested changes Nov 7, 2016

View reviewed changes

Rogier-5 force-pushed the mob-deserialization branch 4 times, most recently from 1de0cf4 to 38d29fd Compare November 9, 2016 14:10

nerzhul reviewed Nov 9, 2016

View reviewed changes

nerzhul added this to the 0.4.15 milestone Nov 9, 2016

nerzhul approved these changes Nov 9, 2016

View reviewed changes

sfan5 requested changes Nov 9, 2016

View reviewed changes

Rogier-5 force-pushed the mob-deserialization branch from 38d29fd to a1ecfb7 Compare November 9, 2016 19:05

Rogier-5 changed the title ~~Fix mob deserialization errors in the client + code factorization~~ Fix mob deserialization errors in the client Nov 9, 2016

Rogier-5 mentioned this pull request Nov 9, 2016

Sao refactor #4760

Closed

sfan5 requested changes Nov 9, 2016

View reviewed changes

Rogier-5 force-pushed the mob-deserialization branch from a1ecfb7 to 05196e9 Compare November 10, 2016 11:20

sfan5 approved these changes Nov 10, 2016

View reviewed changes

est31 approved these changes Nov 10, 2016

View reviewed changes

est31 added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Nov 10, 2016

est31 merged commit 7e17eae into minetest:master Nov 10, 2016

Rogier-5 deleted the mob-deserialization branch November 10, 2016 13:32

Fix mob deserialization errors in the client #4743

Fix mob deserialization errors in the client #4743

Conversation

Rogier-5 commented Nov 6, 2016

Foghrye4 commented Nov 6, 2016 • edited Loading

Rogier-5 commented Nov 6, 2016

Foghrye4 commented Nov 6, 2016 • edited Loading

nerzhul commented Nov 6, 2016

Rogier-5 commented Nov 6, 2016 • edited Loading

Rogier-5 commented Nov 6, 2016

Foghrye4 commented Nov 6, 2016 • edited Loading

Rogier-5 commented Nov 6, 2016

sfan5 commented Nov 6, 2016

Rogier-5 commented Nov 7, 2016

nerzhul commented Nov 7, 2016

Rogier-5 commented Nov 7, 2016

nerzhul commented Nov 7, 2016

Rogier-5 commented Nov 7, 2016 • edited Loading

nerzhul commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerzhul Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rogier-5 commented Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerzhul commented Nov 9, 2016

nerzhul commented Nov 9, 2016

sfan5 left a comment

Choose a reason for hiding this comment

nerzhul commented Nov 9, 2016

Rogier-5 commented Nov 9, 2016

sfan5 left a comment

Choose a reason for hiding this comment

Rogier-5 commented Nov 9, 2016

est31 commented Nov 10, 2016 • edited Loading

Rogier-5 commented Nov 10, 2016

nerzhul commented Nov 10, 2016

Foghrye4 commented Nov 6, 2016 •

edited

Loading

Foghrye4 commented Nov 6, 2016 •

edited

Loading

Rogier-5 commented Nov 6, 2016 •

edited

Loading

Foghrye4 commented Nov 6, 2016 •

edited

Loading

Rogier-5 commented Nov 7, 2016 •

edited

Loading

nerzhul Nov 7, 2016 •

edited

Loading

Rogier-5 commented Nov 9, 2016 •

edited

Loading

est31 commented Nov 10, 2016 •

edited

Loading