Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Writing #24

Merged
merged 41 commits into from
Aug 12, 2024
Merged

Writing #24

merged 41 commits into from
Aug 12, 2024

Conversation

Marlamin
Copy link
Contributor

@Marlamin Marlamin commented Aug 8, 2024

With the public release of https://github.com/ModernWoWTools/DBCD (a formerly private continuation of @barncastle's feature/db_writing branch) I wanted to merge this back into the main repo to avoid an even larger split going forward.

I first merged our master back into @barncastle's feature/db_writing and then manually worked in the changes from the other repo on top of it as there was no intact history on that one.

Massive thanks to @YetAnotherBinarySpace for getting barncastle's old branch across the finish line (for WDC3 at least) and @Luzifix for now open-sourcing it.


Changes

Possibly breaking change for external projects: One of Barncastle's major changes was the renaming of the DBFileReaderLib project to DBCD.IO given it now does both reading and writing. This is likely the largest breaking change.

A large amount of the other changes are member variable name changes present in both Barncastle's branch as well as the ModernWoWTools one, which I chose to adopt (although some old styling remains and will likely need to be cleaned up in a future pass). Some changes also had to be done to keep compatibility with all the frameworks we currently target.

Tests

I have updated and added tests for reading/writing for all 9.2.7.45745 (WDC3) DB2s, all of which appear to pass except for UnitTestSparse.db2 (of which the point is likely to break unit tests) and SummonProperties.db2 which might be a broken DBD that only showed up during writing, oops. (Flags<32> might be Flags<32>[2]).

Note that the tests do not test for client compatibility (yet?) as the generated DB2s will always have a single section (even for source DB2s with multiple sections). I might also test for verifying DB2 byte-equality between official and generated DB2s (for DB2s without multiple sections) but that might end up being a rabbit hole I am not ready for and won't influence PR readiness.

I have not ran complete reading/writing tests on all DB2 versions yet, reading wise there are largely only minor changes, however, writing for the ModernWoWTools repo was only used on 9.2.7 (WDC3) so writing other than that is currently entirely untested and possibly unsupported still depending on version.

Update: All versions except for WDB2-WDB5 (due to missing DBDs) have been tested to at the very least not crash during reading and writing as well as parsed DB2 rows from input DB2s matching output DB2s generated by DBCD.

Hotfix testing as well as making sure there are 0 regressions in reading still needs doing.


This was a large effort to merge, so I'm going to see this through in getting this finished up and merged as soon as possible before I lose the motivation to do so.

🚨 Consider this the call for feedback & heads-up for any breaking changes. 🚨

Testing to do

  • Hotfix applying
Build Version Reading Value match¹ Writing Value match² Byte match³
3.3.5.12340 WDBC ✔️ ✔️ ✔️ ✔️ 244/245
WDB2 ❔⁴ ❔⁴ ❔⁴ ❔⁴ ❔⁴
WDB3 ❔⁴ ❔⁴ ❔⁴ ❔⁴ ❔⁴
WDB4 ❔⁴ ❔⁴ ❔⁴ ❔⁴ ❔⁴
WDB5 ❔⁴ ❔⁴ ❔⁴ ❔⁴ ❔⁴
7.2.0.23436 WDB6 ✔️⁵ ✔️⁵ ✔️⁵ ✔️⁵ ✔️⁵ (1/1)
7.3.5.25928 WDC1 ✔️ ✔️ ✔️ ✔️ ❌ 106/610
8.0.1.26231 WDC2 ✔️ ✔️ ✔️ ✔️ ❌ 109/630
9.2.7.45745 WDC3 ✔️ ✔️ ✔️ ✔️ ❌ 343/830
10.1.0.48480 WDC4 ✔️ ✔️ ✔️ ✔️ ❌ 369/928
11.0.2.56044 WDC5 ✔️ ✔️ ✔️ ✔️ ❌ 417/1011

¹ = matches values with DB2s read by master branch
² = matches values with source DB2s
³ = can only succeed on DB2s with 1 section (or none)
⁴ = not tested due to missing DBDs for this build
⁵ = only Map.db2 tested due to missing DBDs

barncastle and others added 21 commits August 8, 2019 19:54
Original db_writing branch of wowdev/DBCD with changes from ModernWowTools/DBCD on top

Co-Authored-By: barncastle <1619104+barncastle@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: barncastle <1619104+barncastle@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: Luzifix <7042325+luzifix@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: Luzifix <7042325+luzifix@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: Luzifix <7042325+luzifix@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: Luzifix <7042325+luzifix@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: Luzifix <7042325+luzifix@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: Luzifix <7042325+luzifix@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: Luzifix <7042325+luzifix@users.noreply.github.com>
Co-Authored-By: BinarySpace <29510799+YetAnotherBinarySpace@users.noreply.github.com>
Co-Authored-By: Luzifix <7042325+luzifix@users.noreply.github.com>
@Marlamin Marlamin self-assigned this Aug 8, 2024
@YetAnotherBinarySpace
Copy link
Contributor

Great work from your end! Sorry for not upstreaming this myself (as I was planning to do it at some point), quite busy at the moment.

Note that there are a few assumptions that might not be compliant for other versions (aside from WDC3) with Blizzard's reader.
For instance, I spent quite some time to wonder why there was some kind of desync (rows getting mixed up) with copy rows, actually, it was due to the ordering that is necessary (by the id of the copied row). This is a consequence of the internal datastructure backing up the copy rows used by Blizzard (

foreach (var copyRecord in CopyData.OrderBy(r => r.Value))
).

So overall from my experience, the reader is more related in terms of constraints than Blizzard's one so don't be surprised if for some versions it ends up requiring a few tweaks to have a proper behavior in-game.

At least for WDC3, everything should be running pretty smoothly since I tested on my end running with all the DB2s written by the lib and everything was running smoothly in-game.

Furthermore, the heuristic for compression, might be more efficient or more conservative, so indeed don't expect a byte-match, but the logic there should be pretty solid overall.

Also, if you have any questions about astrange behavior or uncommented shenanigans, feel free to poke me on Github (also available on Discord if needed).

@Marlamin
Copy link
Contributor Author

Marlamin commented Aug 9, 2024

Great work from your end! Sorry for not upstreaming this myself (as I was planning to do it at some point), quite busy at the moment.

No worries, I'm glad it got open-sourced and we got the chance to get this in at all.

Note that there are a few assumptions that might not be compliant for other versions (aside from WDC3) with Blizzard's reader. For instance, I spent quite some time to wonder why there was some kind of desync (rows getting mixed up) with copy rows, actually, it was due to the ordering that is necessary (by the id of the copied row). This is a consequence of the internal datastructure backing up the copy rows used by Blizzard (

foreach (var copyRecord in CopyData.OrderBy(r => r.Value))

).
So overall from my experience, the reader is more related in terms of constraints than Blizzard's one so don't be surprised if for some versions it ends up requiring a few tweaks to have a proper behavior in-game.

At least for WDC3, everything should be running pretty smoothly since I tested on my end running with all the DB2s written by the lib and everything was running smoothly in-game.

Yeah, I'm sure the WDC3/9.2.7 stuff works fine (it is noticably separate from the lower versions). There are definitely a bunch of quirks I had to fix on other versions and I'm sure there's many more issues that working with just DBCD itself won't pick up but a client will, but those will reveal themselves in due time as I'm not planning on testing all formats in-game before merging, that'll have to be report/issue-based end user testing. 😋

Furthermore, the heuristic for compression, might be more efficient or more conservative, so indeed don't expect a byte-match, but the logic there should be pretty solid overall.

Haha yeah I noticed that most of the differences I ran into on WDC1/WDC2 are just lower record sizes compared to Blizz. For comparisons sake I might put some effort into figuring out their heuristics/byte boundary targeting just to get some 1:1 tests going to confirm writer integrity. For WDC3+ that's obviously going to be a bit harder without multiple-section support which frankly isn't going to be necessary for community use cases, so I'm probably not going to bother getting any closer with those.

Also, if you have any questions about astrange behavior or uncommented shenanigans, feel free to poke me on Github (also available on Discord if needed).

Will do! Thanks!

@Marlamin Marlamin marked this pull request as ready for review August 10, 2024 22:54
@Marlamin
Copy link
Contributor Author

Hotfixes appear to work in wow.tools.local using this branch and stuff looks fine, so I might finish it up here. I doubt anyone will bother to review this further, so I'll merge it in the next few days.

@Luzifix
Copy link
Contributor

Luzifix commented Aug 11, 2024

I review and tested it, it works fine for me 👍

@Marlamin
Copy link
Contributor Author

I don't fully trust everything yet but there's no good way of testing it all so I'm gonna go ahead with merging this. If any issues come up people can pin/refer to the 1.x tag while any issues are being sorted. The first tag/version with writing support will be 2.0.0.

@Marlamin Marlamin merged commit 9108eeb into master Aug 12, 2024
1 check passed
@Marlamin Marlamin deleted the feature/db_writing branch August 12, 2024 04:43
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.

4 participants