Warpten / DBClientFiles.NET

The new version of DBFilesClient.NET.
GNU General Public License v3.0
11 stars 5 forks source link

Bugfix/wdb5 6 #8

Closed barncastle closed 6 years ago

barncastle commented 6 years ago

Just to summarise:

I also merged with your commits and used them over the top of my fixes.

For testing I added the structures for ItemSparse, WMOMinimapTexture and Achievement (which covers all flag combinations) for both 7.2.5.24393 and 7.1.5.23420 (attached below) - however field names aren't correct. For what its worth I tested several random records from each DB against WDBX and they all matched, hopefully that's a good sign!

(I have a compulsive habit of hitting the auto format keyboard shortcut so if I've butchered your nice alignment just line comment it and I'll fix it - sorry)

Test DBs.zip

Warpten commented 6 years ago

Changed AddFileMemberInfo to accept OffsetMap.Exists as to prevent the MemberInfo.BitSize being set. I did this as having a bitsize was forcing the use of the packed readers which doesn't work with the OffsetMap and it's dastardly variable length inline strings (this needs reviewing)

I think this is fine because if you are dealing with inlined strings, the code throws if it's unaligned.

All in all RecordReader is not a very good solution as there are situations where pushing 32 on the stack for ReadInt32 makes zero sense. I'm however not sure of how to improve on that. All my attempts at having regular-style bitwise readers failed with wrong values being read.

barncastle commented 6 years ago

Sorry that was awful wording on my part.

Setting BitSize fixes the 3 byte int issue however causes a regression in CodeGenerator whereby it forces GenerateBinaryReader to always use the packed reader methods. The problem with the packed reader methods is that it concretes the field offsets, which is a problem for inline strings. After reading an inline string you can't guarantee the following field offsets due to varying strings length.

My solution was to force GenerateBinaryReader to use the standard reader methods however in hindsight I did this in a very bad way and I should've used !Reader.Header.HasOffsetMap in GenerateBinaryReader. (Will fix)

I guess the ideal scenario would be to remove the field offsets from the packed reader methods and use _bitCursor to dictate the current position - this might be an ass of a refactor though?


I don't think RecordReader is bad per-say, my only thought is that you know the offset, you have the data buffered and you know the fields are aligned so you could just do a direct cast (BitConverter, unsafe etc) instead? Something like this (but less shit 😄 )

public long ReadInt64()
{
    long value = BitConverter.ToInt64(_recordData, _bitCursor / 8);
    _bitCursor += 64;
    return value;
}

I'll ping you this evening if you're about - I'm most probably off the mark on some of these issues.

Warpten commented 6 years ago

I wanted to make CodeGenerator<T> inheritable (that's why there are a bunch of virtuals scattered around) but I didn't get around to it. That'll probably help this issue, since it cuts off the need for packed reader tests for older formats