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

Show absolute amount of matching cards instead of percentage value #14

Merged
merged 1 commit into from
May 23, 2017
Merged

Show absolute amount of matching cards instead of percentage value #14

merged 1 commit into from
May 23, 2017

Conversation

ottowayne
Copy link
Contributor

@ottowayne ottowayne commented May 16, 2017

This makes it easier to see how similar the opponent's deck is compared to the selected one.
It shows the amount of cards played from the deck (right number) and the amount of played cards that actually matched the deck chosen by Advisor (left number).

Before:
2017-05-16 12_55_16-hearthstoneoverlay

After:
2017-05-16 12_52_38-hearthstoneoverlay

I am not 100% sure that all cards are counted. I used the IsCreated function to ignore cards that have been added by e.g. spells, but maybe there is more left to check.

@djdookie
Copy link
Owner

djdookie commented May 16, 2017

Hi @ottowayne,
thanks for your contribution.
I like the idea, but I'd prefer to have an option to switch between both presentations, so we can let the user decide.

Some spontaneous thoughts about that:
Since everything is based on the Jaccard (similarity) index, this exact value is displayed yet. And this is imho the best way for similarity calculation and should not be changed.
But I think you're right and it's more intuitive to display the number of matching cards together with the number of played cards instead of that similarity value by default.
One difference is the Jaccard index decreases if played cards are not matching. Initially I thought we would need a third value here, the number of not matching cards, to get the same information.
But since the difference of both numbers easily gives you that information, I support your approach. ;)

After a quick view on the code, I think "!opponentDeckCard.IsCreated" is obsolete, because it's based on opponentCardList, which doesn't contain created cards anymore:
IList<Card> opponentCardlist = Core.Game.Opponent.OpponentCardList.Where(x => !x.IsCreated).ToList();

Regarding jousted (revealed but not played) and discarded cards, your code should also be correct.

I will try to implement and test your pull request and add an option to switch back to the Jaccard presentation in the next days.

Thank you!

@djdookie djdookie merged commit 598d07e into djdookie:master May 23, 2017
@djdookie
Copy link
Owner

djdookie commented May 23, 2017

Now, I included this feature in Release v1.0.5 1e3eeb0
I recognised that your counting was not correct, because you just counted the equalling card names, not the amount of equalling cards. This way, double cards weren't counted correctly.
My implementation went to a new extension method.
Additionally I implemented an option to switch back to the percentage, but the absolute similarity is turned on by default now.
Thanks again!

Cheers, Dookie.

@ottowayne
Copy link
Contributor Author

Thanks for merging!
Another thing I thought about adding was tooltips for the Advisor cards, but that seems to be harder than I originally thought because HearthStoneDeckTracker does not offer generic functions for this.
One idea on how to implement this was to add a whole new Window and copy the functions used by HSDT for its own windows. But that was just an idea so far.

@djdookie
Copy link
Owner

djdookie commented Jun 6, 2017

Yeah, unfortunately there is no out-of-the-box mouseover support for HDT plugins.
I had a look into that very early and decided to skip that for now.
If HDT code changes regarding this or if you find an easy way to do this, let me know.
This feature was also proposed in #13.

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.

2 participants