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

Implement BattleDungeonResult_V2 #131



Copy link

@Lyrex Lyrex commented Mar 14, 2020

This extends the work done in PR #130.
The patch should now correctly keep track of the rewards using the newly added changed_item_list.

Since I did not have logs for secret dungeons and materials, so this still needs some verification. Local tests were successful and indicated that rewards are correctly tracked again.

Fixes: #128

Lyrex and others added 4 commits March 14, 2020 22:50
This is the first, naive try to implement the new BattleDungeonResult_V2
packet implemented by Com2Us.
It seemingly does only add new fields without removing any that we
are using, which is why the existing code that should still work for
the new packet is mostly just duplicated.

Fixes: swarfarm#128
In this commit proper handling for the new structure of the
BattleDungeonResult_V2 api command is added.
Crafting materials and secret dungeons are still untested but should work.
Copy link

I had started collecting test data and modifying the test cases in my own branch, but I put them in here with yours. So far the only failing case is a monster drop. Do you want to add that?

Copy link

Also, it seems that the BattleDungeonResult command is completely depreciated, so it can be removed from the code (schema, parse fn, etc)

Copy link
Contributor Author

Lyrex commented Mar 15, 2020

After I found a secret dungeon today (thank god I didn't have to waste too much time on it) I implemented the missing tests and parsing.
Additionally I could implement the missing monster drop case that you provided a test case for.

Since secret dungeons seem to be a new drop type (item_type 30), I used a magic value for now in the parsing logic. We might want to refactor that into a proper constant, I just wasn't sure where to put it and how to name it.

According to your suggestion I removed the usages of the old and now deprecated BattleDungeonResult referenced too. I kept that in a singular commit to be able to revert it easily if necessary.

One thing you should verify again is the test_monster_drop test case. For me testing sometimes fails for it, sometimes it works. I checked the values in the debugger and ran the query against the database manually and from there everything seems fine. I'm not sure if that's just a test-environment-thing or if it's a real error.

…ngeon drop to item list like other drops. Changed monster drop unit_master_id to something available in the test fixtures.
Copy link

Awesome, I pushed a few small changes and its good to merge! Thank you very much for taking care of this.

@PeteAndersen PeteAndersen merged commit 4e7659c into swarfarm:master Mar 15, 2020
@Lyrex Lyrex deleted the implement-battle-dungeon-result-v2 branch March 24, 2020 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

Dungeon Results are not logged anymore (missing BattleDungeonResult_V2)
2 participants