CivEffects

This task keeps on growing and growing. I just moved map trade from CvTeam to CvPlayer. Being a CivEffect setting, it is linked to players, not teams. Also it is no longer possible to write python code, which enables or disables map trade directly. The plan is that python should be able to enable CivEffects though, which mean map trade can be enabled that way if we really want to.

I noticed that natives are banned from trading maps even if they have the CivEffect needed to do so. I didn't touch that, but eventually that should be removed. At some point after CivEffect is completeted, we should simply search for "isNative()" and replace them with CivEffect settings. We should possibly have a native group in CivEffect where we can customize precisely what we want for each player and it can change during the game.

I wonder how many other settings I will run into, which turns out to be code rewriting rather than simply changing the access function in CvPlayer. Next up is allows civic, which I really want to be an InfoArray rather than an int. That will solve the problems I have with cache init. The same goes for allow trade screens. It's unlikely that we will need to change more than one with a single CivEffect, but adding support for it in the DLL is a trivial addon to the cache init.

EDIT: actually prohibited civics are only checked when changing civic and it only checks against the currently selected civics, which mean one check for each CivicOptionTypes (currently 5). This mean the real solution is to move it to the civic group rather than the allow group and since it's checked that rarely we shouldn't bother with a cache. It's not like it loops the entire array.
 
Yeah, it will probably take both of us to actually get everything working correctly with these changes. But it will assuredly simplify things in the long run.
Yeah I just (intentionally) broke discovering the sea trade route by moving a unit onto the access plot. I updated the first post about this issue and how it might be solved later. I'm not sure I can fix all the issues alone as some of them likely requires knowledge of what you were thinking when you were coding.
 
Added task #15 regarding traits and scenarios. The idea is that CvPlayer can store traits, but if they aren't set, it will use the traits from XML. The DLL will then not set the traits and the leaders will update traits as XML is updated, just like they do right now (or is supposed to do, I haven't checked).

Scenarios should then have the option to fill in traits. If no traits are added, the array is empty and XML is used. If traits are written in the scenario, those traits will be used instead of XML.
 
It's a good thing I look though the CivEffect tags and their implementation. I just renamed two incorrectly named tags (they are now iMaxCityPopulationChanges and MaxStorageChanges). However more importantly it turned out that CvCity wasn't using the data, meaning we could set anything to those tags and nothing would happen ingame.

While correcting this problem, I learned that CvBuildingInfo::getMaxYieldModifiers() didn't take game speed into account. I changed the entire warehouse capacity function to work on normal and then it adjust the result before returning. It makes the code easier to read and faster to execute.

Kailric:
There is a task for you once the XML file has been split into multiple files.

I have a script to move the tags around to match the schema file. However it deletes all tags, which have been renamed and tags, which have been changed from an int into an array would also cause issues. This mean somebody (you) should look through the files and compare them to the current civic XML file to see if important stuff got lost.

I think you would be the best person for this task as it would be natural to review all settings and possibly modify some of them. Also it will give a chance to get familiar with the new options.

EDIT:
I just encountered an issue. As it turns out, hurries has a setup for sacrificing population. Looks like it is copied from BTS, but it lacks an actual implementation. It sounds like a cool addition, but I wonder if it is such a good idea after all because unlike BTS, we care which population unit we sacrifice. Maybe the hurry should have a setting telling which unitclass it sacrifices. That would allow players with slavery civic to sacrifice a random slave to hurry production.

I just extended JIT arrays to call a function whenever an index is updated (in addCache) to make it update the sacrifice population setting and now it's no longer needed :cry:
I wrote it somewhat generic and it could be a useful feature for other settings. I will just let it stay even if it is unused for now.

The whole allow hurry code is not as well coded as the other allow code and it isn't actually used due to XML settings. I will make it comply to the standard and then it might actually be used. It could be interesting to make a CivEffect, which disallows the buying production with gold, and then allows another buying with gold, where each hammer is cheaper. The same goes for immigration. Also we can make a CivEffect, which completely disables your ability to hurry immigrants if we want to.
 
I updated the first post with a counter for implementation. It's currently at 23 out of 83 tags, which are fully implemented and functioning ingame.

Reviewing the implementation of each tag turns out to be really important as some of the tags aren't implemented perfectly. In some of them are missing... an implementation :eek:

Bugs fixed today:
  • Cities were unaffected by iMaxCityPopulationChanges
  • Cities were unaffected by MaxStorageChanges
  • City storage took game speed into account for some changes, but not all
  • When opening the window where "the king" asks for a fee for transporting a treasure, the pope would receive tax payment from the treasure even before you replied. You would pay tax again if you said yes. Now you will not pay until you actually accept and you only pay once.
  • Hurry couldn't be disabled (such as hurry production or hurry immigrant)
    Now it's actually possible to have a CivEffect, which disables hurry production and then enables another hurry production, which has a different price for each hammer.
Nice list of fixes, which came as an unplanned bonus from reviewing the code :)

Ok, just say when.
It could take a while and it might be quick. I have no idea, but I'm planning ahead. I have a vision of the finished feature, which will hopefully avoid unforseen problems.
 
I came up with a new idea. Eras shouldn't have a field for a CivEffect they enable. They should BE CivEffects themselves. I would dare to say that adding Eras as CivEffects can be done in less than 10 lines in C++ and by adding the tag groups in XML schema (that's around 8 lines). It's a minor change in the DLL, but it sure can impact greatly on the gameplay :)
 
While working on unit heal rate, I realized I could add more useful tags for this with very little work. Each tag was added with just one line of code in addition to the cache+xml info code. New tags are in bold.
iHealRateEnemyUnitsModifier | Increases heal rate to ENEMY units in player's plots (will likely be used with negative modifiers)
iHealRatePlotsEnemyModifier | Increases heal rate in plots where you are at war with the owner
iHealRatePlotsFriendlyModifier | Increases heal rate in plots owned by your team
iHealRatePlotsNeutralModifier | Increases heal rate in plots, which aren't in any of the previous two categories
Now this part of the code feels more complete and while I don't have any ideas for which CivEffects should use those tags, we do have them and they can easily be activated once somebody figures out a good way to use them.

I don't really like the way the different bonuses from this and other sources stack. However that can be recoded without affecting XML files and can be done later. Recoding this isn't a 5 minute job as it is affected by multiple XML values, CivEffects, promotions and possibly other stuff as well.

I had another idea for a CivEffect tag. Added movement costs to enemy units in your plots, possibly one for neutral units as well. This will really make border spreading interesting as it will make it even more efficient at avoiding surprise attacks.

Damage enemy units each turn on your plots is also an interesting one, though it would be quite powerful if used with too high numbers.

Good deal, yeah if I helped you with this I would need to make sure I knew which ones you were working on. Anyway, I'll have more time to look into the code tomorrow and I will talk to you online before I even attempt to help with this.
There might not be a need for that. I'm no longer tired of implementing the cache and I can get a lot done again. Maybe my brain decided to take the weekend off or something :lol:
 
Moving along nicely, I have now gone through 2/3 of the tags.

I implemented ProfessionCombatChanges, through right now when I write this, I realized I implemented it as a modifier :wallbash:
However I'm considering removing it again because also after I implemented it, I realized we could do the same with free promotions for professions (one of the things in the remaining 1/3).

I changed chance for goody huts to a default of 100 for each and recoded the code to decide which one it is. You can now change the default with GoodyFactors. If you end up reducing the score to 0 or below that, the goody will be turned off for that player.

I completely changed iImprovementUpgradeRateModifier. I added ImprovementUpgradeRateModifiers, which is an array of improvements and you can mod speed for each. iImprovementUpgradeRateModifier will then loop that array and apply bonus to all, meaning XML can mod improvements individually or all in a single line.

I changed the behaviour as well. Rather than changing how many points you apply each turn, it mods how many points is needed to upgrade. Default is 1 point meaning vanilla can't change more than multiplying the speed. If the upgrade takes 50 turns, it mean based on bonus it can take 50, 25, 13, 10 and so on. Nothing in between. If we increase speed by 10%, it's still just one point each turn. However if we reduce the points needed, we can reduce 10% and it will take 45 turns. In other words adding small bonuses will make sense.

I needed a speed cap for technical reasons and since it's faster to divide with n^2 than any number not following that, I decided to divide by 4 and cap at -75% (turns rounded down). This mean an upgrade, which takes 50 turns is capped at 12 turns. However increasing time is uncapped and you can make it 300 turns if you want to play nasty :devil:

Because an upgrade time of 0 would be... interesting, I added an assert saying that upgrade time has to be at least 4 (as this caps the improved speed at 1). The actual assert message is this:
IMPROVEMENT_WHATEVER upgrades in 2 turns (min 4 required)
replace 2 with whatever it written in XML
That should be pretty clear of what went wrong when reading the XML file even without looking at anything else and this got me thinking. Why don't we have asserts like that for all XML read functions? If you violate the restrictions for what you can write in XML, you are clearly not aware of the restriction (or senile :old:) and such a message would make you know what to do even without having to ask on the forum.

Now I want to review all CivEffect reading code and add asserts like that. Some tags have restrictions (like you can't modify speed or cost to less than -100%). The less unreported XML errors we have, the less bug reports we will get for working DLL code.

Another thing I realized is that iImprovementUpgradeRateModifier is a poor name now that the function is changed. Shouldn't it be iImprovementUpgradeTimeModifier?
 
Another thing I realized is that iImprovementUpgradeRateModifier is a poor name now that the function is changed. Shouldn't it be iImprovementUpgradeTimeModifier?

Sure :)

Well, that's all good. Just remember to get some exercise every few hours :run:
 
Competition time. What is wrong with the following code?
Code:
for (int i = 0; i < GC.getNumCivicInfos(); ++i)
{
	if (this->getIdeasResearched((CivicTypes)i))
	{
		CvCivicInfo kInfo = GC.getCivicInfo((CivicTypes)i);
		applyCivEffectCache(kInfo, 1, false, true);
	}
}
I had to search the entire DLL for this issue. You get to examine a few lines. Let's see if anybody can spot it.

Road movement bonus
After close examination of (road) movement cost, I have finally figured out completely how it works.

Moving from one plot to another has 3 options for costs and it will use the lowest:
  1. Terrain cost * MOVE_DENOMINATOR (this includes features, hills and so on)
  2. route cost
  3. route flat cost * unit movement/turn

Actual moves for each unit per turn is moves per turn * MOVE_DENOMINATOR. This allows fractions of movement points.

MOVE_DENOMINATOR is 60 by default, which is actually a really good number because it can be int divided without rounding errors into 1, 2, 3, 4, 5, 6, 10, 15, 20 and 30. In fact the ancient Babylonians discovered this and based their entire number system on this to make fractions easier and it is also the reason why we have 60 minutes in one hour.

The problem is that it is not a good system for XML setup. In fact it is rather confusing and not used correctly right now.

Proposal:
Removing flat cost. I find it rather confusing to have two route movement bonuses and I don't quite follow the logic in it. Also it can remove some code in CvPlot::movementCost(), which is good as it is called quite often.

The the XML interface to write how many plots the unit can move using a single movement point. CivEffect can then modify this using simple numbers like +1 and -1.

Current modifier stacking:
modifiers|cost|plots
0|30|2
+10|20|3
+10+10|10| 6:eek:
+10+10+10|0| 60 :crazyeye:
Note: the movement cost is capped at 1 meaning there is a hardcoded limit of moving max 60 plots on one movement point.

Proposed modifier stacking:
modifiers|cost|plots
0|30|2
+1|20|3
+1+1|15|4
+1+1+1|12|5
This makes it easier to stack bonuses as well as just simply setting up the unmodified XML files.

This will require a DLL cache where the XML numbers are converted into actual cost numbers. This isn't an issue and with the current CivEffect cache, it will perform at the same speed as it is right now.

Vanilla has support for not using the route on the plots if the terrain has less movement costs. I'm considering redesigning that. If there is a valid route, then the unit will use that and entirely skip calculating terrain movement costs. This will produce faster code and I can't really see we could end up in a case where the route is slower than the terrain.

There is just one drawback from redesigning this system. If it is redesigned, we move away from vanilla, which has been an established standard for 10 years. I'm not sure if that is an issue or not.

Should we go for it and ditch the vanilla route system?

EDIT:
I just had a new idea. Instead of 60, we could split each movement point into 12252240 parts. This number is calculated to allow integer division of all numbers in the range 1 to 18 meaning we will not get rounding errors until we have a route, which allows 19 plots/move. 20-22 would work though. This system would overflow if a unit has more than 175 moves, which should be enough.

The tradeoff is that it is near impossible for humans to handle numbers that big, but if we only use them inside the DLL and not in XML, that shouldn't really be a problem, except if we need to debug this. This mean the code should be written in a way that allows to easily switch to 60 for human readable debugging. Alternatively we should just write the code well enough to not need debugging ;)
 
Hmm, CvCivicInfo kInfo is it missing the & sign and should be CvCivicInfo&. It just looked naked, not 100% sure what the & does exactly.

About the road proposal. I'd say to add it to a list of proposed code improvements and perhaps make that a release all in one where we do tons of code improvements.
 
Hmm, CvCivicInfo kInfo is it missing the & sign and should be CvCivicInfo&. It just looked naked, not 100% sure what the & does exactly.
& mean that it is a reference to a variable rather than a variable itself. It is sort of related to a pointer.

There are two ways to code this:
  1. Don't use references
    This is bad because the class has no copy constructor. It just copies the pointers.
  2. Use references
    No local data is used. It will read from the memory of the original one. Great since we aren't changing the data.

What appears to happen here is that the get function returns a reference, which is then assigned into a non-reference. At the end of the {}, the copy goes out of scope, calls the deconstructor and frees the memory. The info class now contain bogus data, hence why I read that a tech allows profession 744 :crazyeye:

This is a serious issue as it really wrecks the code if the & is missing and there are no warnings or anything about it, which mean the error will show up next time we read from the info class in question, not where the bug occurred.

The only thing I can think of to prevent this is to rewrite the get functions into returning const pointers. The compiler will stop compiling if the * is missing and it will also stop compiling if we try to change something in a const pointer, meaning the info class is fixed data once it is set, which is precisely what we want. Rewriting into const pointers isn't the most fun task, but it is a whole lot more fun than hunting a bug caused by this.
 
Top Bottom