Dynamically reevaluate first round picks during trades

[sumitde22]

A second go-around of trying this "trade AI should reevaluate the draft pick values they are giving up when changing team OVRs". It doesn't change the cache this time (uses some extra storage, but doesn't modify what was being stored before), just does some reevaluation, haven't seen much of a difference in speed for "Ask for Trade Proposals" when testing.

Testing Conducted: For reference, here are the starting team ratings: https://imgur.com/5zubF50

On the master branch, teams are willing to give up 1st round picks even when their team OVR tanks to < 40: https://imgur.com/abL2ZIZ

One team is willing to do this trade that tanks their team OVR to -36 while also giving up 2 immediate 1sts. Not to mention also give up a ton of value in players: https://imgur.com/V8ZbWJj

On the feature branch, teams are not willing to tank their OVR as much when giving up 1sts, the lowest I see is 50 OVR which would be around the 10th pick: https://imgur.com/iSpNqeQ. Generally the trades look more realistic to me.

The same team from before is willing to give up much less but still make what I consider a pretty reasonable trade: https://imgur.com/9QMRfue

[dumbmatter]

The biggest performance concern is with AI-to-AI trades, because they have to run the trade finding logic many times to find a valid trade. So the concern is that this might slow down how long it takes to sim a season, especially if the AI trade frequency is increased. I tried to test but get this error when I sim games on this branch:

    at Cache._indexGetAll (Cache.ts:877:22)
    at async getModifiedPickRank (unknown:627:3)
    at async Object.valueChange (unknown:584:21)
    at async attempt (betweenAiTeams.ts:111:14)
    at async Object.betweenAiTeams (betweenAiTeams.ts:158:26)
    at async cbSaveResults (play.ts:165:5)
    at async cbSimGames (play.ts:532:3)
    at async cbPlayGames (play.ts:586:4)
    at async cbRunDay (play.ts:612:6)
    at async Object.play2 [as play] (play.ts:645:4)

Also if we were going to recompute estimated picks for every different trade, cache.estPicks seems useless and should be deleted, I'm not sure why it's still sometimes used.

[sumitde22]

Sorry about that bug, I got a little tunnel vision on fixing valueChange and didn't realize it was also being called for AI-to-AI trades and FA re-signing, and getModifiedPickRank assumed that tradingteamtId would always be available

I pushed a few changes so that it retrieves the players differently, also removed the calculation of estPicks.

I simmed a few seasons to make sure any errors wouldn't pop up and tried some speed tests as well.

Three auto-played seasons on the branch resulted in

Auto play done in 38.052 seconds, Auto play done in 41.5 seconds, Auto play done in 39.883 seconds

Three auto-played seasons on master resulted in

Auto play done in 39.234 seconds, Auto play done in 39.616 seconds, Auto play done in 36.221 seconds

so speed seems ok on my end, but I'm sure you have a much wider range of cases/settings you want to verify with

Let me know if there's other things I should fix/change!

[sumitde22]

Just attempted to add some speedups to see if that helps the performance issues, if it doesn't I'll try to see if there's a maintainable way to write this for just basketball leagues

[dumbmatter]

Doesn't seem to have helped, performance is similar. However, I did some profiling and realized that reformatting the players array before passing it to team.ovr is very slow, and can be improved by allowing team.ovr to support raw player objects with no reformatting. That actually seems to get rid of most of the performance difference between branches (while also making team ovr calculation faster in other situations, which is nice).

Apparently I have commit access to your branch, so I just pushed a commit with that change :)

If you look in betweenAiTeams.ts, those 3 commented lines are good for performance testing. Uncomment them and run await bbgm.trade.betweenAiTeams() on the worker console and it'll simulate a ton of AI trades and tell you how long it took.

I would appreciate it if you could test this code and make sure I didn't break something in team ovr calculation or trade value calculation!

Then I also wonder if the binary search is actually speeding things up. Can you benchmark that and see? If it helps, keep it. If not, get rid of it.

And could you handle the merge conflicts, by merging the master branch into this branch?

(Feel free to say "no" to either of those questions, and then I will do it! You have already helped a lot!)

This also highlights that there are probably other needlessly slow parts of my code that I just haven't looked at in depth yet...

[sumitde22]

Yes to all these questions. Your commit is actually really helpful as I didn't realize that was a bottleneck, kinda just took the function that already existed, so thanks!

When I was doing some profiling locally, I realized that team.ovr was being called about 10x as much as on the master branch, which feels untenable for the performance of your game based on your comments before about how calling .ovr this much during the season when finding AI-to-AI trades could really slow it down. I'll incorporate your fix, but what I've done locally (haven't pushed yet) is set up a bunch of approximations/regressions and caching to vastly reduce the # of calls to ovr (actually less than the master branch version) while still maintaining high accuracy.

I will incorporate your latest commit into my local branch and fix the merge conflicts. I also have a bunch of ideas for testing since its getting to the point where its non-trivial change in trade AI, but I'll let you know about that once I have more of a finished product for this working

[dumbmatter]

Thanks! In the long run it'd be good to have unit tests showing that certain trades are accepted/rejected, to make it less likely that a bug in the trade logic is introduced. But that doesn't have to be done now.