Core: Rewrite start inventory from pool code

[NewSoupVi]

I think the old code is pretty ugly and unpythonic

Here's my proposal for a better version

Even had some room to spare for comments, since I made it a couple lines shorter

[Exempt-Medic]

I feel like this section could all be inside an if overall_target: and then at the end have:

else:
    for player, target in target_per_player.items():
        new_itempool += [multiworld.worlds[player].create_filler() for _ in range(target_per_player[player])]

[NewSoupVi]

I do think this makes the code slightly more complicated, at probably little performance gain (The full iteration of the multiworld itempool is gonna be so much slower than anything that happens after it)

But I see your point. I'll think about it. I don't like the idea of just wrapping it in an extra indent, but maybe there is a different cute solution here.

[NewSoupVi]

I timed it roughly, this change barely has any impact. The bottleneck is the exact code you're isolating:

    for player, target in target_per_player.items():
        new_itempool += [multiworld.worlds[player].create_filler() for _ in range(target_per_player[player])]

These lines take the most amount of time (after multiworld itempool iteration, which is the slowest part of the code).
The additional calculations you're avoiding with this change, barely make a dent.
I think in this case, I'd prefer to keep the code shorter / simpler, but open to be convinced still

[Exempt-Medic]

Yeah, I went and timed it myself with 10K players and it saved 0.0054 seconds on average over 20 trials (the range was 0.005-0.006) so... yeah, not worth

[NewSoupVi]

I made a different improvement that should make this better in the worst cases

[Exempt-Medic]

One comment, otherwise the changes LGTM. Tested various configurations of start_inventory_from_pool and all behavior was identical to how it was before.

[Exempt-Medic]

New changes look good, did some more testing (including two cases I forgot about which would have failed previously...) and everything seems to work well.

[NewSoupVi]

I have now opted to remove overall_target.

I understand why it exists, not doing it is about 20% slower in the "average worst case" (lol), which could be considered worth it for a full iteration of the multiworld itempool.

However, it

1. Makes the code more confusing and complicated
2. Full multiworld itempool iterations, and things that are way worse than it, are done elsewhere
3. Now that we have this "warn instead of crash" behavior, I imagine in big multiworlds, statistically, one player will make use of it, and as soon as one item can't be found, we're in the bad code path anyway

I can be convinced to bring it back, though.

[qwint]

code looks correct and cuts some corners that should improve performance but didn't actually run the code yet to see if i can catch any issues (medic seems to have ran through most of them tho lol)