zengm-games / zengm

Basketball GM (and other ZenGM games) are single-player sports management simulation games, made entirely in client-side JavaScript.
https://zengm.com
Other
361 stars 131 forks source link

Use "after trade team ovr" when evaluating draft pick value in trades #434

Closed sumitde22 closed 1 year ago

sumitde22 commented 1 year ago

This is a follow up PR to a minor "issue" I found here.

The TL;DR is it would make sense to me that if a CPU team is evaluating their draft pick's value and is using team ovr as part of the calculation, it should based on the after trade team ovr, not the before.

I tested this locally on insane mode, and it seems to be a little improved in my view. On master https://imgur.com/8XJoVsJ On the branch https://imgur.com/7maEFC2

I basically offered a valuable prospect and saw what a CPU might offer back in default. In the master, the CPU offers 2 1sts including the current year despite gutting their team ovr to like -66 or something, which is a lock for a top 5 pick despite the CPU valuing it as a late 1st. On the branch, the CPU only offers 2nds in this case instead when offering all the players, which seems subtle but I think is a big difference: picks around ~30 rather than picks in the top 5.

I'm sure you have preferences for how you access the cache or the type of data you store in places. This is my first time really looking the codebase so I just added a field to an existing data type, but feel free to suggest doing it totally differently if it looks too hacky

sumitde22 commented 1 year ago

Also another small "bug" I tried to fix in the third commit: I found that the CPU is willing to trade releasable players (who at worst should have 0 value) for negative value contracts, allowing the user a loophole to rid themselves of a bad contract.

On master: https://imgur.com/wxaCzGZ

WAS and PIT both willing to give away releasable players for negative contract

On branch: https://imgur.com/TAUb13y

No teams willing to give releasable players for bad contract

Forgot to mention in description, but I also ran the unit tests on this branch and they all passed

dumbmatter commented 1 year ago

Thanks, these both seem like good ideas! I will try to merge this soon.

Two things I'm going to try changing first:

  1. Simple one - move justDrafted/wasJustDrafted to common, so there is no duplicated code.

  2. Tricky one - as you guessed, ovrAfter should be computed somewhere else, and should not rely on trade.summary being called because that's really just to generate stuff to display in the UI. I think it might actually be fine to just dynamically compute it inline... it's a bit worse that way because then you can't share code with the ovrBefore calculation in trade.summary. But I don't think we actually gain much by caching it.

dumbmatter commented 1 year ago

Actually it might be worse either way... currently it's caching team ovrs in memory (refreshCache) so it doesn't have to recompute them every time it evaluates valueChange, which happens a lot when the AI is searching for a trade (like a counter offer, or trades between AI teams). With your current code, that won't work right - it's still just setting the cached team ovrs once. Instead, it'd have to not cache team ovrs at all, and always dynamically compute them, and then recompute pick values from that, for every hypothetical trade. I suspect that would wind up being too slow, because that's why I added refreshCache in the first place. Although feel free to test it if you want!

The draft stuff is probably still worth doing though. You can either make a separate PR wtih just that (if you want me to merge in your PR) or if you'd rather not do that, lmk and I'll do it in my own commit.

Also please make sure you're okay with the code license https://github.com/zengm-games/zengm/blob/master/LICENSE.md and send me a copy of the CLA https://github.com/zengm-games/zengm/blob/master/CLA-individual.md

sumitde22 commented 1 year ago

Just made two separate PRs for these two changes as you mentioned