Skip to content

Fix ClientboundEntityUpdateAttributes key mapper: missing attributes shift every subsequent attribute's name - #1254

Open
reallyoldfogie wants to merge 2 commits into
PrismarineJS:masterfrom
reallyoldfogie:fix/entity-attribute-mapper-missing-attributes
Open

reallyoldfogie wants to merge 2 commits into
PrismarineJS:masterfrom
reallyoldfogie:fix/entity-attribute-mapper-missing-attributes

Conversation

@reallyoldfogie

Copy link
Copy Markdown
Contributor

Fix ClientboundEntityUpdateAttributes key mapper: missing attributes shift every subsequent attribute's name

Problem

packet_entity_update_attributes (Java Edition's ClientboundEntityUpdateAttributes) sends each entity attribute as a VarInt id plus a value and modifiers. Since Minecraft 1.20.5, that id is a plain sequential registration-order index into the game's attribute registry — it is not a stable enum.

For every PC version from 1.21.1+, the mapper for this list was missing several attributes that exist in the real game registry:

• burning_time, explosion_knockback_resistance (added early in 1.21)
• mining_efficiency, movement_efficiency, oxygen_bonus, sneaking_speed, submerged_mining_speed, sweeping_damage_ratio, water_movement_efficiency (added in 1.21)
• 1.21.5 additionally had a bogus camera_distance entry that doesn't exist in that version's real registry at all.

Because the mapper is just a flat, indexed list with no version-range checks, every attribute registered after the first gap gets silently mislabeled — the packet still decodes fine (no parse error, no panic), it just reports the wrong attribute name and the correct value/modifiers attached to it. Concretely, on the affected versions, the real generic.movement_speed index was being read back as generic.step_height (1.21.1/1.21.3/1.21.4) or generic.scale (1.21.5 through 1.21.11/latest/26.1) — meaning any consumer reading attributes by name got either the wrong attribute's data, or nothing, for every attribute at or past generic.max_health in the registry.

Root cause

The mapper lists in proto.yml are hand-curated and were evidently never updated in lockstep with the game's own attribute registry as new attributes were added across the 1.21+ line. This is distinct from attributes.json, a separate reference data file (name/min/max/default, not used by the packet decoder) that — for every affected version — already lists every attribute in the correct, real registration order. That gave a reliable, in-repo source of truth to correct the mapper against, without needing to trust any external decompiled-source claims.

Fix

For each affected version, the key: varint => list under packet_entity_update_attributes in proto.yml was reordered/extended to exactly match that version's own attributes.json order (with the minecraft: prefix stripped). Two rules were followed throughout:

  1. Never rename an attribute key already present in a given version's mapper, even where the real id has since changed — existing consumers may depend on that name. Only missing entries were inserted, in their correct position, shifting subsequent indices up.
  2. Where a version's naming convention had already changed between attribute groups (some older attributes still use generic./player./zombie. prefixes; attributes added from 1.21 onward do not), newly-inserted entries follow that same version's own convention rather than inventing a new one.

The one exception to rule 1: 1.21.5's fabricated camera_distance entry (which doesn't exist in that version's real registry) was removed and replaced with the real attribute at that slot, burning_time — this is a data-correctness fix, not a rename, since camera_distance was never a real 1.21.5 attribute.

Why these versions, and not others

Every PC version's protocol definition for this packet was checked to establish the exact boundary of what could be affected:

• Through 1.20.4: the attribute key is sent as name: string — a literal resource-location string, not an index. There's no sequential mapper at all, so this class of bug is structurally impossible.
• 1.20.5 / 1.20.6 (they share one proto.yml): this is where the index-based key: varint => mapper was introduced, but its attribute list is already complete for that version — none of the attributes missing in the 1.21.x bug existed in the game yet. Not affected.
• 1.21 / 1.21.1, 1.21.3, 1.21.4, 1.21.5, 1.21.6, 1.21.8, 1.21.9, 1.21.10, 1.21.11: affected, per above. (1.21 shares 1.21.1's files; 1.21.10 shares 1.21.9's.)
• latest (and therefore 26.1, and any future version until the next branch-off): has the identical 4-entry gap as the 1.21.6+ group. Left unfixed, every future version bump would inherit the same bug, so it's included even though the original bug report only checked through 1.21.11.
• No proto/protocol files exist for any version between 1.21.11 and 26.1, so there's no gap in coverage there.

@extremeheat extremeheat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Automated review from Codex, requested by @extremeheat.

The overall diagnosis and most of this patch look correct, but I found one remaining registry-order issue that should be fixed before merging.

For both Minecraft 1.21.3 and 1.21.4, vanilla registers the final entries in this order:

  • 29: sweeping_damage_ratio
  • 30: tempt_range
  • 31: water_movement_efficiency

The proposed maps omit tempt_range and place water_movement_efficiency at ID 30. Because water_movement_efficiency is actually ID 31, it would still decode incorrectly in those versions. This also means the maps do not yet exactly match those versions' attributes.json files, which include tempt_range.

Vanilla references:

Please add tempt_range immediately before water_movement_efficiency in the 1.21.3 and 1.21.4 proto.yml files and regenerate/update their protocol.json files accordingly.

Everything else I checked against the extracted vanilla sources looks good, including the 1.21.5 camera_distance removal and the 1.21.6+ ordering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants