Revert the 1.21.6 enum mappers whose ordinals never moved - #1292
Conversation
`difficulty.difficulty`, `set_difficulty.newDifficulty`, `game_state_change.reason` and `map_chunk.heightmaps[].type` became mappers in 1.21.6 while staying plain numbers in every earlier protocol. The wire values behind them have not changed in any version, so the mapper buys no stability and only costs compatibility: - Reading, `difficulty` now yields a string, so mineflayer's `difficultyNames[packet.difficulty]` is `undefined` on 1.21.6+ and `bot.game.difficulty` is unset. - Writing, a numeric value throws since ProtoDef-io/node-protodef#176 made the compiled mapper reject values not in the mappings (before that it fell through to the raw value, which is why these went unnoticed). That breaks flying-squid's `game_state_change {reason: 2}` and any client writing `set_difficulty {newDifficulty: 0..3}`. Reverted to the exact pre-1.21.6 types, so existing code needs no change. `difficulty`/`newDifficulty` go back to `u8`: vanilla writes the enum ordinal, and for a 4-value enum that is byte-identical to the varint they were given here. `entity_action.actionId` deliberately keeps its mapper: the two sneak actions moved to `player_input` in 1.21.6, so every ordinal after them shifted by 2 and a numeric value really does mean different things per version. `teams.mode`, `update_structure_block.flags` and `client_command.actionId` (PrismarineJS#1284) are left for separate changes.
|
This is actually a pretty simple thing to fix, might do it in a bit |
VasilisDragon
left a comment
There was a problem hiding this comment.
Both difficulty fields should stay varint. From 1.21.6, vanilla uses Difficulty.STREAM_CODEC through ByteBufCodecs.idMapper; u8 only happens to match the bytes for 0–3.
This also changes existing named writes: newDifficulty: 'hard' now writes 00 instead of 03. Numeric writers recover, but named callers would need migrating.
|
Nominally, varint vs u8 should only matters for values >128 as the 4 MSBs are unused on both However if clients/servers write invalid values that intentionally are out of an enum's bounds, this can cause serializer exceptions if using u8 in place of something that's supposed to be varint32 (upto 5 bytes) |
|
Yeah, agreed. Numeric 0–3 encode the same. I should've been clearer that this was about matching vanilla's |
Reverts four fields that became
mappers in 1.21.6 while staying plain numbers in every earlier protocol, back to their exact pre-1.21.6 types:difficulty.difficultyu8mapper(varint)u8set_difficulty.newDifficultyu8mapper(varint)u8game_state_change.reasonu8mapper(u8)u8map_chunk.heightmaps[].typevarintmapper(varint)varintThe wire values behind all four have never moved, so the mapper buys none of the insertion-stability a mapper is for, and it is currently breaking consumers in both directions:
difficultynow yields a string, so mineflayer'sdifficultyNames[packet.difficulty]isundefinedandbot.game.difficultyis unset on 1.21.6+.game_state_change { reason: 2 }anddifficulty { difficulty: 0..3 }, and any client writingset_difficulty { newDifficulty: 0..3 }.difficulty/newDifficultygo back tou8rather than thevarintthey were given here: vanilla writes the enum ordinal, and for a 4-value enum that is byte-identical to a varint, sou8matches the other 52 pc versions exactly.Touches
data/pc/{1.21.6,1.21.8,1.21.9,1.21.11}/proto.ymlanddata/pc/latest/proto.yml, withprotocol.jsonregenerated vianpm run build.npm testpasses (1860 passing).Deliberately not reverted
entity_action.actionIdkeeps its mapper. The two sneak actions moved toplayer_inputin 1.21.6, so every ordinal after them shifted by 2 (start_elytra_flyingwent 8 → 6). Here a bare number really does mean different things per version, which is exactly the argument in Fix pc/26.1 client_command actionId: plain varint, not a mapper #1284 for naming these — reverting it would turn mineflayer'sactionId: 2inbed.jsfrom a loud throw into a silentstop_sprinting. ItsentityActionUsesStringMapperfeature flag stays valid.teams.modeis entangled with a real 1.21.6 packet restructure (flatmode ? if 0:switches → an anonymous_switch,friendlyFire: i8→ aflagsbitflags). Not a revertable line.update_structure_block.flagslanded in 1.21.5, not 1.21.6, and is mismodelled independently of this: a bitfield expressed as a 4-entry enum mapper.client_command.actionId(Fix pc/26.1 client_command actionId: plain varint, not a mapper #1284) anduse_entity.hand(Fix pc/26.1 use_entity hand: plain varint, not a mapper #1278) are already in flight.After this, those four are the only remaining fields in the pc data that are a
mapperin some versions and a raw number in others.On the wider direction
@extremeheat — this cuts against your suggestion on #1284 of mapper-ising everything, so to be explicit about where I think the line is: I agree names are the right long-run representation, and the insertion-stability argument is real (
entity_actionabove is a live example of it). What makes the 1.21.6 batch a problem isn't the direction, it's that it landed per-version on fields whose numbers were never unstable, which means consumers pay a migration for no stability gain and end up with version-gated branches like mineflayer'sentityActionUsesStringMapper.If we do want to go wide, the sequencing that would make it painless is worth doing first:
mapperto accept the underlying wire value on write as well as the name — i.e. after the name lookup fails, accept the value if it is itself a key ofmappings. An unmapped name still throws, so add PDS file for 1.11 #176's guarantee is untouched, but mapper-ising a field stops being a breaking change on the write side.writeEnum/readEnumcall sites give ordinal order mechanically, and minecraft-data-generator could emit anenums.json.Happy to do (1) and (3) as follow-ups if that sounds right.