bedrock: Incorrect NPC packet serialization

[Bluebotlabz]

These packets and information were taken from version 1.19.31
It is possible, yet unlikely, that this is specific to this version

There is a possible packet with request type of 6:

{"runtime_entity_id":"13","request_type":6,"command":"","action_type":"set_actions","scene_name":""}

It appears to be the opening equivalent of execute_closing_commands execute_opening_commands
As NPC are capable of executing commands when they are interacted with

Additionally,
NPC packets with request type of 1 (or "execute_action") are parsed incorrectly, according to the documentation action type is an enum, however, when request_type is 1 (or "execute_action") it is actually an integer referring to the button which was pressed, starting from 0, which corresponds to the leftmost button

note: despite being an int, the client limits the number of possible buttons you can add to 6 buttons, so action_type will never be higher than 5 if the request_type is 1

Similar behaviour is observed when request_type equals 4 (or set_skin) in which the action_type actuall corresponds to the NPC's set skin (again, counting from 0, left to right) the highest action_type for setting the NPC's skin is 34 in the current update, however, this may change in the future

Lastly, I believe that the notes for the Command field name are somewhat misleading as it isn't necessarily a command
ie: when setting dialogue, it is equal to what is being typed and in set_actions it is equal to a JSON object containing all the possible NPC actions (it effectively override the action related entity data)

NOTE: All of this data is from the Bedrock edition of the game, while the Education Edition may behave in the same way, I have no way of confirming this, however, it is likely that it does

[Bluebotlabz]

Interestingly enough, in

minecraft-data/data/bedrock/latest/proto.yml

Line 2207 in ca72acc

6: execute_openining_commands

You can see that action_type of 6 is valid in the enum, but not request_type of 6

[Bluebotlabz]

Also, there is an action_type 2 for packet_npc_dialogue

I don't know what it does though It appears to close it

[extremeheat]

I don't see any problem here. If id 6 is returned by the server and we don't have a string mapping for it, it will (as intended) return the raw integer read from stream. There are no conditional fields in this packet so the reading of one field will not impact that of any other.

[Bluebotlabz]

I believe that it should be mapped to a string called "execute_closing_commands" "execute_opening_commands"

[extremeheat]

Where are you sourcing that change ? Link to GitHub ?

[Bluebotlabz]

I would create a pr with minecraft-data, however, I am unable to test it locally as I am having problems with npm installing it when I did clone it...

I'll send some packet data in a second that pretty much confirms that id 6 is closing commands

[extremeheat]

Can you provide a buffer then ? I will test it when I get the free time. We have CI for basic checks but it's not exhaustive in checking individual packets. Posting a Github link to an external implementation doing it as you suggest would be helpful to base the changes from. Moving this to minecraft-data.

[Bluebotlabz]

Ok, somehow I have made a typo all throughout this issue, id 6 is execute_opening_commands

I'm not sure how I didn't catch that, but I'll edit it now

I'm not too sure how to export a buffer, so I'll just send a JSON packet log with the required packets when I can

[extremeheat]

Looks like this will be fixed in #715 - https://github.com/PrismarineJS/minecraft-data/pull/715/files#diff-dde1740bd800507b5353cd54ab82c249e8f3b8206a8290c791370278a61cadcdR2219

[Bluebotlabz]

That looks like it fixes it