collinsmith / riiablo

Diablo II remade using Java and LibGDX
http://riiablo.com
Apache License 2.0
872 stars 99 forks source link

Implement proper item serialization #93

Closed collinsmith closed 3 years ago

collinsmith commented 3 years ago

Item serialization has been placeholder and needs to be properly implemented. Currently item serialization data is just a reference to the data as it was deserialized (i.e., as it was within the d2s). This issue will require implementing support within BitStream for writing values. Ideally the deserialized format should match the original serialized format to maintain compatibility.

collinsmith commented 3 years ago
See #95 I've been researching and playing around with various APIs regarding support for this issue. I've decided to settle on creating `com.riiablo.io` package since this spec is growing and have separate classes for input and output. Reading and writing in the same stream should not be required by Riiablo, and doing it this way seems easier. Looking into `bit-io` library for strategy -- some good ideas, but not completely sufficient for my needs (slicing buffer). I may also couple the I/O package with netty `ByteBuf`, since it supports pooling and is compatible with network I/O (eventually I'd like to have all codecs use `ByteBuf`, pending research). Long term, if it's not too expensive, I'd like to look into using this for all I/O. This would mean that all codecs use the same code base, and more pertinently, when a codec needs to create a slice for a section, it can do so easily. The bit input API will also support recovery at a specified signature -- which will be helpful if the codec can recover and log a warning instead of a fatal error. I'm also going to guarantee that byte position can be retrieved, so tracking down errors will report offset within file (this gets a little messy when inside something like an item bit stream, so offset would probably be the start of item's input slice).
collinsmith commented 3 years ago
See #95 dd1444ad9a143fe4f67debd1df828615e7847115 created `com.riiablo.io` package. I'm happy with this API so far -- supports seamless reading from bit stream (aligned and unaligned), enforcing alignment only when using no-args read methods. The idea is that bit stream read operations should use methods that specify bits, while byte stream operations should use no-args methods. This may cause issues down the line if some bit stream read operations use no-args methods when they just happen to be aligned (I can foresee these as a nightmare to track down). Alternatively, I could modify the API to work as follows: ``` ByteInput align() {...} align().read8u(); BitInput unalign() {...} unalign().read7u(7); ``` This would probably require a lot of duplicate code though. I could also try a state machine within `BitInput` to manage if we're in byte or bit mode. I like this idea perhaps the most, but it also seems very error prone -- maybe a bit better if I create specific methods for like reada32u to read an aligned 32 bit unsigned int which will throw invalid state if called and in unaligned mode. -------------------------------------------------------- First idea -- ugly code because this is a junit test body ``` ByteInput bytes = ByteInput.wrap(new byte[] { 0x4A, 0x4D, 0x10, 0x00, (byte) 0x80, 0x00, 0x65, 0x00, 0x04, (byte) 0x82, 0x26, 0x76, 0x07, (byte) 0x82, 0x09, (byte) 0xD4, (byte) 0xAA, 0x12, 0x03, 0x01, (byte) 0x80, 0x70, 0x01, 0x01, (byte) 0x91, 0x03, 0x01, 0x04, 0x64, (byte) 0xFC, 0x07}); Assert.assertArrayEquals(new byte[] {0x4A, 0x4D}, bytes.readBytes(2)); // signature Assert.assertEquals(0x00800010, bytes.read32()); // flags Assert.assertEquals(101, bytes.read8u()); // version BitInput bits = bytes.unalign(); bits.discard(2); // unknown Assert.assertEquals(0, bits.read7u(3)); // location Assert.assertEquals(0, bits.read7u(4)); // body location Assert.assertEquals(2, bits.read7u(4)); // grid x Assert.assertEquals(0, bits.read7u(4)); // grid y Assert.assertEquals(1, bits.read7u(3)); // store location Assert.assertEquals("hbw ", bits.readString(4)); // code Assert.assertEquals(0, bits.read7u(3)); // sockets filled Assert.assertEquals(0x2555A813, bits.readRaw(32)); // id Assert.assertEquals(6, bits.read7u(7)); // ilvl Assert.assertEquals(4, bits.read7u(4)); // quality Assert.assertEquals(false, bits.readBoolean()); // picture id Assert.assertEquals(false, bits.readBoolean()); // class only Assert.assertEquals(0, bits.read15u(11)); // magic prefix Assert.assertEquals(737, bits.read15u(11)); // magic suffix bits.discard(1); // unknown Assert.assertEquals(32, bits.read15u(8)); // max durability Assert.assertEquals(32, bits.read15u(9)); // durability Assert.assertEquals(57, bits.read15u(9)); // poisonmindam Assert.assertEquals(0, bits.read31u(0)); // poisonmindam param bits Assert.assertEquals(8, bits.read31u(10)); // poisonmindam value Assert.assertEquals(0, bits.read31u(0)); // poisonmaxdam param bits Assert.assertEquals(8, bits.read31u(10)); // poisonmaxdam value Assert.assertEquals(0, bits.read31u(0)); // poisonlength param bits Assert.assertEquals(50, bits.read31u(9)); // poisonlength value Assert.assertEquals(0x1ff, bits.read15u(9)); // stat list finished Assert.assertEquals(5, bits.bitsRemaining()); // tail end of stream ``` ----------------------------------- Regarding state machine idea, sample code would be something like below. Should allow using same object for all methods without having the syntax above. Method names become a big uglier to help enforce use-cases. Some methods will enforce aligned state, some unaligned, and changing between states is only possible through explicitly calling align/unalign, so calling an unaligned method in aligned state will throw an error and not accidentally unalign stream. I'm iffy on allowing methods that don't care about state -- I like the idea of forcing the code to be explicit, but readability is nice too. Some methods would need to be renamed because supporting multiple effects based on state will be more error-prone (i.e., calling `discard(bits)` when you really meant to call `discard(bytes)`). ``` BitInput b = ... // aligned by default if wrapping bytes b.reada32(); b.discardUntil(...); b.reada16u(); b.read16u(); // iffy on this name, since it's byte-aligned size, could go either way b.unalign(); b.reada16u(); // would throw state exception, even though stream is aligned, because read mode changed to unaligned b.read15u(8); b.read31u(24); b.align(); ```
collinsmith commented 3 years ago

Basic serialization is complete. Going to tighten up code where I can and push.

collinsmith commented 3 years ago

Serialization tested on a large variety of items, so far so good. I'm going to start working on deprecating/removing cache variables such as Item#data so I can close this issue.

collinsmith commented 3 years ago

Fixed by 2beecb35a8f7ef1d9b5c43c368f63c9dd51bef3f. Item#data issue will be addressed by #98