- #842 Small performance optimisations for the BuyStuff JoyGiver#843
- #842 Small performance optimisations for the BuyStuff JoyGiver#843simplyWiri wants to merge 2 commits intoOrionFive:developfrom
Conversation
| { | ||
| var selectedThings = new List<Thing>(); | ||
|
|
||
| // For performance ... doesn't necessarily mean we are actually doing this randomly :) |
There was a problem hiding this comment.
Won't this mean they'll buy the same things all the time?
There was a problem hiding this comment.
Yes - You can do something like randomize the index within the array of items - and wrap around if you reach the end (incrementing n times) if you would like that behaviour. I'd drop the second commit and re-write if you are interested, the first commit can be CP'd directly, imo.
Second commit is just an example of why this function is (still) slow after my first change, and offers a potential approach to making it faster. I don't think its a huge deal - but with large shopping areas, this causes the game to stutter noticeably.
There was a problem hiding this comment.
I don't understand what the idea behind the wrapping around is. I do understand the need for fixing the performance, though.
I find it quite important that they make a random selection. So if you could commit a fix for that, it'd be great. I suppose with this code it shouldn't be an issue if duplicate elements are picked by random, so that should allow for a fairly cheap solution.
There was a problem hiding this comment.
Basically, you have some (semi-ordered) list things. And you want to select n random items from the list.
We want to avoid calling the IsValid check super frequently, and if we do - lets try to have it in semi-contiguous memory. To choose a random item, we can pick a random starting index, and increment from there, if our random index, is say - the second last index. We then wrap around to the start of the array again.
This ensures that (worst case) we still check every item in things, but in the best case - we can early out much faster and dramatically reduce the number of IsValid calls.
There was a problem hiding this comment.
There was a problem hiding this comment.
Can you add some fail safes? I feel if things is empty or less than 5, we might get issues. Or have you tested that?
|
Thanks for making this PR. However, I'm concerned they'll only ever check out the same items. |
2 participants
This whole class is written in a way which is only friendly to very small shopping areas, with highly restricted number of cells.
For the sake of performance there is an argument to be made for picking items in the region first, then randomly selecting a few, and only buying them if you are able - instead of getting a "validated" list up-front. I've done this locally for my own sake, as otherwise this causes the game to stutter for a tenth of a second, every second, effectively making the game 10% slower when I have guests on my map.
However, obviously the above is a change in behaviour - however, the approach of using a lazy approach to select items is not without merit - if you group all items, then have some lazy function like
.RandomWhere(t => CanBuyThing(t))you would have a much faster algorithm in general, especially when there are lots of things that a given pawn can buy.