Added the missing WrapperPlayServerMapData #760

ShadowOfHeaven-Me · 2024-05-07T21:47:31Z

The title explains it

booky10

there are a few issues with this packet wrapper, especially regarding version compatibility

also, is this class being packaged in shadow.utils.holders.packet.custom intended?

booky10 · 2024-05-08T15:37:35Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+                if (hasDisplayName) writeComponent(icon.getDisplayName());
+            }
+        } else if (version.isOlderThanOrEquals(ServerVersion.V_1_19))
+            writeVarInt(0);//The array is not optional on 1.19 and below, so if it's missing, we have to at least write that it's length is 0


its also optional in 1.19 and below, this is wrong

It's not wrong, in 1.19 and below you at least the array's size needs to be written if it's empty, unlike 1.19.2 and above.
https://wiki.vg/index.php?title=Protocol&oldid=17753#Map_Data 1.19
https://wiki.vg/index.php?title=Protocol&oldid=18067#Map_Data 1.19.2

but I guess the comment is "present only if previous boolean is true", so I'll delete that part

wiki.vg is wrong there, there wasnt a single change to the packet between 1.19 and 1.19.2

it changed between 1.16.5 and 1.17 and you also dont account for it properly (you still write the optional boolean), see other comment #760 (comment)

see https://haste.pinofett.de/9n8fvvep52.diff for diff in decompiled source between 1.16.5 and 1.17

booky10 · 2024-05-08T15:38:18Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+                writeVarInt(icon.getType().getId());
+                writeByte(icon.getX());
+                writeByte(icon.getZ());
+                writeByte(icon.getDirection());


you should maybe add & 0xF, like done in vanilla

also, this is only present since 1.13 - in older versions this is OR-ed into the first value (see comment above)

booky10 · 2024-05-08T15:39:16Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+            int length = this.readVarInt();
+            MapIcon[] icons = new MapIcon[length];
+            for (int i = 0; i < length; i++)
+                icons[i] = new MapIcon(MapIconType.ofId(this.readVarInt()), this.readByte(), this.readByte(), this.readByte(), this.readBoolean() ? this.readComponent() : null);
+            this.icons = icons;


This can be converted to an readList call, if you change the icons field to List<MapIcon>

booky10 · 2024-05-08T15:39:47Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+            int length = this.readVarInt();
+            MapIcon[] icons = new MapIcon[length];
+            for (int i = 0; i < length; i++)
+                icons[i] = new MapIcon(MapIconType.ofId(this.readVarInt()), this.readByte(), this.readByte(), this.readByte(), this.readBoolean() ? this.readComponent() : null);


the type can be read using this.readMappedEntity(MapDecorationTypes::getById)since 1.13 (see last comment for reason)
before that, this is just a byte: the first 4 bits determine the type (mask 0xF0) and the last 4 bits determine the direction (mask 0x0F)

you should maybe add & 0xF after the last this.readByte(), like done in vanilla
also, this is only present since 1.13 - in older versions this is OR-ed into the first value (see comment above)

the name can be read using this.readOptional(PacketWrapper::readComponent)
also, this is only present since 1.13

maybe split this up into multiple lines with comments stating what you're reading

booky10 · 2024-05-08T15:40:17Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+            writeVarInt(icons.length);
+            for (MapIcon icon : icons) {


This can be converted to an writeList call, if you change the icons field to List<MapIcon>

booky10 · 2024-05-08T15:51:50Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+
+        private MapIconType type;
+        private byte x, z, direction;
+        private Component displayName;


there is an @Nullable annotation missing here

booky10 · 2024-05-08T15:51:54Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+            this.type = type;
+        }
+
+        public Component getDisplayName() {


there is an @Nullable annotation missing here

booky10 · 2024-05-08T15:51:58Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+            return displayName;
+        }
+
+        public void setDisplayName(Component displayName) {


there is an @Nullable annotation missing here

booky10 · 2024-05-08T15:53:07Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+    public enum MapIconType {
+        WHITE_ARROW,
+        GREEN_ARROW,
+        RED_ARROW,
+        BLUE_ARROW,
+        WHITE_CROSS,
+        RED_POINTER,
+        WHITE_CIRCLE,
+        SMALL_WHITE_CIRCLE,
+        MANSION,
+        TEMPLE,
+        WHITE_BANNER,
+        ORANGE_BANNER,
+        MAGENTA_BANNER,
+        LIGHT_BLUE_BANNER,
+        YELLOW_BANNER,
+        LIME_BANNER,
+        PINK_BANNER,
+        GRAY_BANNER,
+        LIGHT_GRAY_BANNER,
+        CYAN_BANNER,
+        PURPLE_BANNER,
+        BLUE_BANNER,
+        BROWN_BANNER,
+        GREEN_BANNER,
+        RED_BANNER,
+        BLACK_BANNER,
+        TREASURE_MARKER;
+
+        public int getId() {
+            return this.ordinal();
+        }
+
+        public static MapIconType ofId(int id) {
+            return id < MapIconType.values().length && id >= 0 ? MapIconType.values()[id] : null;
+        }
+    }


this thing should not be added, there is already a registry for this: https://github.com/retrooper/packetevents/blob/ba21fa9a8b1055f8398d94c196c1d6a79cf04fbf/api/src/main/java/com/github/retrooper/packetevents/protocol/item/mapdecoration/MapDecorationTypes.java

booky10 · 2024-05-08T16:12:56Z

...ain/java/com/github/retrooper/packetevents/wrapper/play/server/WrapperPlayServerMapData.java

+    //The only change occurred here, where the array's length became optional
+    //https://wiki.vg/index.php?title=Protocol&oldid=17753#Map_Data 1.19
+    //https://wiki.vg/index.php?title=Protocol&oldid=18067#Map_Data 1.19.2
+
+    //hasIcons can also be called trackingPositions, but both ultimately do the same


this is wrong

Yep. I'll trust you'll handle the appriopriate changes, since you seem more acquainted with the version changes of this packet

retrooper · 2024-05-09T17:33:32Z

@ShadowOfHeaven-Me Simplest is if you grant @booky10 permission to work on the PR (your fork), and booky if you have time you can work on it, if not I would. Let me know booky. 👍

booky10 · 2024-05-09T17:35:22Z

I can also PR to his fork

retrooper · 2024-05-09T17:40:27Z

If he grants you permission to write to it.

booky10 · 2024-05-10T15:25:37Z

@ShadowOfHeaven-Me I've created a PR on your fork: ShadowOfHeaven-Me#1

Fix map data wrapper version compat

Added the missing WrapperPlayServerMapData

01e161d

booky10 suggested changes May 8, 2024

View reviewed changes

retrooper assigned booky10 May 9, 2024

booky10 and others added 3 commits May 10, 2024 18:00

Fix version compatibility for map data packet wrapper

5001b21

Fix flag parsing for <1.13

9f27a49

Merge pull request #1 from booky10/fix/map-data-wrapper-version-compat

67855ed

Fix map data wrapper version compat

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the missing WrapperPlayServerMapData #760

Added the missing WrapperPlayServerMapData #760

ShadowOfHeaven-Me commented May 7, 2024

booky10 left a comment •

edited

booky10 May 8, 2024

ShadowOfHeaven-Me May 9, 2024

ShadowOfHeaven-Me May 9, 2024

ShadowOfHeaven-Me May 9, 2024

booky10 May 9, 2024

booky10 May 9, 2024

booky10 May 8, 2024

booky10 May 8, 2024

booky10 May 8, 2024

booky10 May 8, 2024

booky10 May 8, 2024

booky10 May 8, 2024

booky10 May 8, 2024

booky10 May 8, 2024

booky10 May 8, 2024

booky10 May 8, 2024

ShadowOfHeaven-Me May 9, 2024

retrooper commented May 9, 2024

booky10 commented May 9, 2024

retrooper commented May 9, 2024

booky10 commented May 10, 2024

Added the missing WrapperPlayServerMapData #760

Are you sure you want to change the base?

Added the missing WrapperPlayServerMapData #760

Conversation

ShadowOfHeaven-Me commented May 7, 2024

booky10 left a comment • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrooper commented May 9, 2024

booky10 commented May 9, 2024

retrooper commented May 9, 2024

booky10 commented May 10, 2024

booky10 left a comment •

edited