Shorts, Threes, and Ints aren’t unsigned

[sorokya]

Noticed while testing this with reoserv that the client reads shorts, threes, and ints as signed values.

This became an issue when reoserv tried to send a a session id that was bigger than a signed short can hold.

I tried fixing this myself but it quickly became a huge change to over 100 files and broke a lot of stuff.

[ethanmoffat]

Yeah this is a big problem. I also went through and tried to update it to use signed values and had the same issue, 100+ files changed quickly became a headache.

The actual problem is that because NumberEncoderService takes an int, it will sign extend shorts that are greater than short.MaxValue. See: b80ff5c

What I ended up doing to resolve it is just casting to ushort client-side. It's an ugly hack but it works.

Which session id specifically was causing the issue?

[sorokya]

In reoserv I generate a random short value between 0 and the max2 value.

https://github.com/sorokya/reoserv/blob/async-rewrite/src/player/player.rs#L154

the error was coming up during the welcome message packet. The session id reoserv sent was too big.

[ethanmoffat]

I'll see if I can reproduce it against reoserv. That commit I linked (b80ff5c) was working for me with larger player IDs on GameServer and etheos

[sorokya]

Ah you’re right casting it back to unsigned does fix it. I’ll just do that for now in my branch. :P

edit:
Doesn't look like that will work if you need to use AddShort.

Using AddThree((ushort) SessionID) works fine because you can fit a ushort in an int but you can't do AddShort((ushort) SessionID).

In #150 I did a dirty workaround by just adding an overload to PacketBuilder AddShort(ushort s)

Let me know what you think :)

[ethanmoffat]

I didn't have that issue for shorts in my testing. I also added a line to PacketBuilder that casts the short to ushort prior to calling EncodeNumber with it so it doesn't get sign-extended during encoding:

EndlessClient/EOLib/Net/PacketBuilder.cs

Line 70 in 5fa705f

return AddBytes(_encoderService.EncodeNumber((ushort)s, 2));

I forgot I updated it in both these places, but it was working consistently for me on both etheos and gameserver

[sorokya]

You're right that totally fixes it. No need for the overload :)

[ethanmoffat]

When playing on BU, this problem is pretty apparent once you get over TWO_BYTE_MAX HP:

Adding the ushort cast fixes it but we need a more universal solution because that's gross and hacky.

[ethanmoffat]

I'm going to take the eolib-java approach and make everything an int instead of using exotic types.