pc/26.1: map every InteractionHand field to main_hand/off_hand - #1278
Conversation
|
Confirmed on a live 26.1 server (JartexNetwork, 1.8 backend behind ViaVersion). Worth noting how it fails, because it does not look like a serializer error from the outside: the throw happens in With this change |
…the same for client_command.actionId
|
Context, in case you'd rather go the other way: mineflayer has since been fixed to write the mapper name ( What's left is the inconsistency itself — inside pc/26.1, |
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Reviewed at the maintainer's request. I inspected the current diff and discussion and locally reproduced named-value serialization before/after with ProtoDef 1.19.0. The inline finding concerns silent behavior changes for callers using the existing named API; no live-server test was run.
| } | ||
| } | ||
| ] | ||
| "type": "varint" |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Removing this mapper silently changes an existing hand: 'off_hand' write from byte 01 to 00 (main hand). I reproduced this with ProtoDef 1.19.0 using the changed field type: its varint writer coerces both names to zero, so this is a wrong-hand interaction rather than a useful migration error. Could we keep the named representation and adapt numeric callers, or supply an explicit compatibility path before removing it? Please cover both named hands in a serializer regression test; testing only main hand hides the regression.
There was a problem hiding this comment.
Agreed, reworked in the other direction (237e8a8): use_entity.hand keeps its mapper, and open_book, arm_animation, block_place and use_item get the same main_hand/off_hand mapping, so the named form is used for every InteractionHand field in 26.1. tools/js/test/protocolHands.js serializes both named hands for each field and checks off_hand → 01. A plain varint writes 00 there, so the test catches the regression you found.
Keep use_entity.hand as a mapper and give open_book, arm_animation, block_place and use_item the same mapping, so all five InteractionHand fields in 26.1 read and write the same names. edit_book is left alone: its field is a hotbar slot index in vanilla, not a hand. A protodef serializer test covers both named hands for each field; a plain varint writes off_hand as 00.
u9g
changed the title
|
Correction to my earlier comment: PrismarineJS/mineflayer#4066 and #4076 were closed without merging, so mineflayer master still writes |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Looks ready to merge from this review. The revised direction preserves use_entity's mapper and adds matching names to the other four hand fields in both YAML and generated JSON. The five added tests pass. Separately, using the complete candidate schema in an isolated process with NMP 1.68.0 / ProtoDef 1.19.0, I encoded and decoded all five full packets: named and numeric main/off hands produce identical bytes (10 pairs), and off_hand remains 1. This avoids the earlier interpreter-versus-production confusion. Current CI is green; no live-server action was claimed.
Skills used: prismarine-protocol-data-review traced the maintained data through its production codec/consumer; prismarine-review checked current revisions, CI and existing discussion to avoid duplicate findings.
CI on this PR (which tests the merge with master) failed "26.1 / protocol.json is desynced from yaml". The 26.2 commits on this branch froze data/pc/26.1/proto.yml from pc/latest and repointed 26.1 at it, but master has since changed 26.1's protocol.json by editing pc/latest/proto.yml (PrismarineJS#1278 InteractionHand main_hand/off_hand mappers, PrismarineJS#1295 entity_action enum names). The merge took master's newer JSON alongside this branch's older frozen yaml. Applied master's pc/latest/proto.yml diff since the merge base to pc/26.1/proto.yml (it was an identical copy) and regenerated. 26.1's protocol.json is now byte-identical to master's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings the frozen 26.2 and 26.3 proto.yml in line with what master did to pc/latest (and what the previous commit ported to 26.1): - PrismarineJS#1278: `hand` on open_book, arm_animation, block_place and use_item is now a main_hand/off_hand mapper instead of a bare varint. - PrismarineJS#1295: entity_action actionId names follow Mojang's ServerboundPlayerCommandPacket.Action (leave_bed -> stop_sleeping, start/stop_horse_jump -> start/stop_riding_jump, open_vehicle_inventory -> open_inventory, start_elytra_flying -> start_fall_flying). Wire format is unchanged: numeric writes still pass through the mappers, and the start/stop_sprinting names are the same. The resulting packet definitions are identical to 26.1's. mineflayer's elytraFly still sends 'start_elytra_flying' under entityActionUsesStringMapper, which is the same break master already has on 26.1 and is handled by PrismarineJS/mineflayer#4120. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
In pc/26.1,
use_entity.handis amapper(main_hand/off_hand), while the otherInteractionHandfields (open_book,arm_animation,block_place,use_item) are plain varints. That means a client has to write'main_hand'for one packet and0for the rest.This PR keeps the named form and applies it to all five fields. That fits the move toward enum mappers in #1295, and it avoids the silent regression flagged in review: with a plain varint, ProtoDef writes
off_handas00.data/pc/latest/proto.yml:open_book,arm_animation,block_placeanduse_itemget the samemain_hand/off_handmapping asuse_entity;protocol.jsonis regenerated.edit_book.handis left as a varint because vanilla'sServerboundEditBookPacketcarries a hotbar slot index there, not anInteractionHand.tools/js/test/protocolHands.js(newprotodefdevDependency): for each of the five fields, serializes bothmain_hand→00andoff_hand→01, and reads01back asoff_hand.Callers that write numeric ids to the four newly mapped fields will now get a serializer error, not a wrong hand.