Hi karadoc,
Could you add a tag on github so that we can easily download the 1.34 source code? The latest tag is for 1.33. Many thanks.
You're right, I forgot to push the new tag. But in any case, the most recent commit on github is version 1.34. You can confirm this by checking the commit message. It says "This is version 1.34." Every version I upload on this site has a commit message like that, so if the tag is missing, then that's a good thing to look for.
(I'll probably just push the tag there when I upload the next version, which I expect will be soon.)
First, just a really minor bugfix. In CvTeam::doesImprovementConnectBonus the FAssert checks look like this:
Code:
FAssert(eImprovement <= GC.getNumImprovementInfos());
FAssert(eBonus <= GC.getNumBonusInfos());
<= is incorrect, it should be <
That's true. The asserts are wrong. Thanks for pointing it out. (Non-modders be reassured though, this has no effect on the mod whatsoever. FAssert stuff is only used when debugging.)
I also notice that CvTeam::doesImprovementConnectBonus doesn't check whether the bonus has been revealed or not. getNonObsoleteBonusType doesn't check for that either. Is the assumption that the tech to reveal a bonus always comes before the tech to connect it?
getBonusType checks whether the bonus is revealed; and getNonObsoleteBonusType uses that function before checking whether the bonus is obsolete. So, the revealedness does get checked.
As for doesImprovementConnectBonus, I was in two minds about whether or not it should check that or not; and I decided just to have it not check it, mostly just so that it's a slightly shorter computation... I figure that it is usually used in conjunction with getNonObsoleteBonusType when checking which improvement to build or what to pillage and so on; and that it may be helpful to
not check whether the bonus is revealed if the player is trying to evaluate a new type of improvement or something like that.
On another note: I have a couple of performance improvements in Realism: Invictus that might be useful to K-Mod:
- PlotGroup Bonus Efficiency
- PlotGroup City Cache
- Living Team Member Cache
- Unit Upgrade Cache (would be improved to handle some edge cases before merging)
- Building Activation
At some point I'd like to add a living player cache to CvGame as well, just to cut down on the bazillion loops through all players checking for aliveness. We have MAX_PLAYERS of 56 in our game core so those loops start to pile up.
Anyway, do you have an interest in having some of these improvements merged in at some point?
-Josh
I do have some interest... In general, performance improvements are good; obviously. The downsides are that they may introduce bugs or maintenance problems, or just be time consuming to implement.
The main thing that puts me off other people's performance improvements is that I had a nightmarish time with the attitude cache from the "CAR" mod which I inherited from the original BBAI code when I first started this mod. That code was trash. It was written in a way that created horrible maintenance problems and it had heaps of subtle OOS bugs. I spent many hours of debugging due to that code; and I recently ended up just writing it completely to be rid of the problems once and for all.
I don't want a repeat of that situation... and so I'll have to read the changes myself carefully before merging them, to make sure they're done the right way; but that's kind of boring and time consuming and blah blah blah.
The bottom line is that I'm interested, but only when there are clear gains and the changes are easy to understand.
--
That said, I know that the upgrade cache is probably worthwhile, and wouldn't be surprised if your plot group stuff was a significant improvement as well. And I'm pretty sure that RI has decent quality-control on these things...
I'm not really sure what you mean by
Living Team Member Cache, or
Building Activation. Checking whether or not a player is alive is essentially instant; and counting the number of living team members in a team is pretty fast too, and I don't think it happens terribly often anyway. But by the sounds of things, what you're talking about has something to do with using something smaller than MAX_PLAYERS when doing most of the loops in the game.
I know there are a
lot of loops in the game which go through all of MAX_PLAYERS, when all they really want to do is go through all of the living players. -- So I suppose you're talking about having a kind of dynamic MAX_PLAYERS thing? I would be interested in doing something like that.
There are a few different way to approach it though.
One way would just be change all of the loops from MAX_CIV_PLAYERS, to some new number, say iNumAlivePlayersEver; or something like that. (Loops with MAX_PLAYERS, as opposed to MAX_CIV_PLAYERS, would need special treatment if they were to be shortened as well, because of the barbarian player always being the last player.)
But maybe you'd want to go a step further than simply changing the size of the loops. There are lots and lots of bits of information in the game which are storied for all players regardless of how many players are actually alive. So if MAX_PLAYERS is 52 rather than just 17, then the memory usage of the game will be significantly higher... It might be nice if MAX_PLAYERS itself could be dynamically set at the start of the game, perhaps based on the map size; and then most of the memory wouldn't have to be allocated in the first place. -- Awhile back, I started implementing something a bit like that. ie. changing all of the allocations to be based on a variable MAX_PLAYERS rather than a fixed number. Statically allocated arrays became vectors and so on. I reckon it could probably be made to work, but it's a significant undertaking...
Is that the kind of thing you're talking about?
