search

unrealities / MTGA_Draft_17Lands

Magic: The Gathering Arena draft tool that utilizes 17Lands data
MIT License
26 stars 1 forks source link

merging fixes and tests #1

Closed FiYir closed 3 months ago

FiYir commented 3 months ago

It looks like you're taking over this project, so I thought I would reach out to you to see you're interested in some changes that I made. I've had a fork of Truth's fork for a while now and I've been adding test cases and fixing broken features. It looks like Truth abandoned his fork, so I figured I might merge with yours.

My Changes

  1. I added support for the new log events from the recent Arena update. My approach to this issue was a bit different from yours, and I decided to try to make the change backward compatible and potentially future-proof. WOTC has reverted changes in the past, so it's possible that they might revert these ones. I verified my changes for premier, quick, and traditional drafts (see tests/test_card_logic.py), but not sealed.
  2. I fixed the Chrome plugin, which had stopped working a while ago. I also modified the UI change a bit to make it clearer that this is not part of the 17Lands site.
  3. I fixed some broken application code related to tier comments and split cards.
  4. I changed the limited_sets code so that when you download a set, you also download the bonus sheet, special guest, list, etc. This was an existing feature in the original project that I believe was used for downloading cube datasets. I just made the change for all of the sets. If you delete your old Temp/temp_set_list file, the app will create a new one with the modified set codes.
  5. I refactored the code so that the latest versions of PIL and Pydantic are supported.
  6. I added GitHub workflows for running the pytest test cases whenever a pull request is created. It includes workflows for Windows, macOS, and Ubuntu.

Truth's Changes

  1. They addressed an issue with the original tool where it wouldn't download card data if the ALSA or ATA values were null (below the confidence threshold).
  2. Replaced the old card compare autocomplete with a custom version. You now use the up and down arrow keys to cycle through matches.
FiYir commented 3 months ago

I needed to make some additional changes to the test_app_update file to pass everything after the merge.

image

I made the limited_sets change for MKM, but I just confirmed that it still works for OTJ. As you can see in the image below, the Big Score, Special Guest, and Breaking News cards are showing up. As stated in my earlier comment, you need to either delete the temp_set_list file and have the app build another, or you can just edit your current file and change the arena field to "ALL".

image

image

If you have any questions about what I had changed, feel free to add them to this request and I will try to answer them.

unrealities commented 3 months ago

I just noticed this. Thank you so much! I'll take a look and get it in the next release.

unrealities commented 3 months ago

Thank you for adding/updating the tests. I'll run them and update them as needed in the future.

I made a change in v3.12 because I broke Quick Draft. There is still a "Payload" section in the pack log.

I see your comment about the bonus sheets and am trying to understand your approach compared to what I did in 3.13. Your approach is obviously ideal, but why did the original author set it up like they did if they didn't need to?

FiYir commented 3 months ago

I see your comment about the bonus sheets and am trying to understand your approach compared to what I did in 3.13. Your approach is obviously ideal, but why did the original author set it up like they did if they didn't need to?

You're hardcoding the codes for the set, whereas the original app automatically supported new sets. My change simply modifies this process using a code option that already existed. I'm not sure if you'll want to support this tool with every new release, which you will have to do with your approach.

There's a tradeoff when using the "ALL" code. If the individual set codes are in the temp_set_list file, the app builds a dataset that only includes the cards from those sets. However, if you use the "ALL" code, the tool retrieves a list of card names from 17Lands and constructs a dataset with every instance of those cards on Arena. Using the individual codes would result in a smaller dataset and file. Additionally, The List doesn't seem to have a set code in Arena, so you wouldn't be able to build a dataset that includes those cards without using "ALL".

unrealities commented 3 months ago

I'm going to merge this in and get a new release updated. You've done a lot of work for the community!

unrealities commented 3 months ago

Included in the latest release: https://github.com/unrealities/MTGA_Draft_17Lands/releases/tag/MTGA_Draft_Tool_V0314