Unofficial Patch for 3.19

I'd say there is a problem. Because yes, as you say, there are lots of other rounding stuffs in the game. However, while the base value of science, gold, hammer... can take high values and make the rounding almost non-perceptible, trade routes almost always have a very low value (1 commerce at the beginning) which makes rounding a big lose a good chunk of the total commerce. If at least all trade routes were added before applying modifiers I would not complain, but the way the game is now people will think they get some benefit from a harbour while in fact they get none.
 
I'd say there is a problem. Because yes, as you say, there are lots of other rounding stuffs in the game. However, while the base value of science, gold, hammer... can take high values and make the rounding almost non-perceptible, trade routes almost always have a very low value (1 commerce at the beginning) which makes rounding a big lose a good chunk of the total commerce. If at least all trade routes were added before applying modifiers I would not complain, but the way the game is now people will think they get some benefit from a harbour while in fact they get none.

If my understanding is correct, the trade routes are in fact added up before they are rounded. Since there are no other other ways for base commerce to get rounded except for Bureacracy capitals, it's only really a big loss when you have 1 trade route at like _.75:commerce:.

The other reason it's really noticeable is that it will happen straight away with your first trade route. At least with hammer rounding and food rounding it doesn't start happening until you start obtaining modifiers through your own actions.
 
Trade route calculations work like this:

Code:
for each trade city
    x = max( base trade rate times 100 with trade city, 100 )
    x = x * (trade rate modifier)
    x = x / 10000

So, rounding is done after multiplication by trade modifiers, however the modifiers are applied on a route by route basis and not on the sum. Given the way trade route profits are shown in game, this is to be expected.

A building which gives +50% trade modifier gives no benefit until the city it's built in has a trade route with a base rate times 100 of 134.

Here's how base trade rate is computed (it returns the base rate times 100):

Code:
int CvCity::getBaseTradeProfit(CvCity* pCity) const
{
	int iProfit = std::min(pCity->getPopulation() * GC.getDefineINT("THEIR_POPULATION_TRADE_PERCENT"), plotDistance(getX_INLINE(), getY_INLINE(), pCity->getX_INLINE(), pCity->getY_INLINE()) * GC.getWorldInfo(GC.getMapINLINE().getWorldSize()).getTradeProfitPercent());

	iProfit *= GC.getDefineINT("TRADE_PROFIT_PERCENT");
	iProfit /= 100;

	iProfit = std::max(100, iProfit);

	return iProfit;
}

The value is XML driven and is the minimum of a population based rate and a distance based rate.
 
jdog, is that the same for EmperorFool's Fractional Trade Routes mod? I maybe didn't make it clear but my comments above were targeted only at that mod. I'm aware that in the unmodded game the trade routes are valued exactly as they are displayed in the trade routes box.

http://forums.civfanatics.com/showpost.php?p=8442961&postcount=175
Some background first perhaps:

  • All arithmetic is done using integers. When non-integral values are needed (e.g. commerce), the value is stored "times 100" to allow for exactly two decimals. This allows faster calculations while allowing for an exact fractional amount.
  • All yields :)food:, :hammers:, and :commerce:) are stored and added up as whole numbers. The only place a fractional yield occurs is in this new feature, and it's only fractional while adding up the trade routes for a single city. The total for a city is rounded as soon as it gets converted into actual :commerce: (and :food: and :hammers:, but in BTS the trade route yield modifiers for those yields are both 0%).
The storage and calculation of fractional trade routes aren't the problem; I've completely handled them with no issues. The only issue arrises is when displaying the values to the user not aggregated city-by-city. When displaying the total trade with a single rival, I must add up a subset of trade route yields across cities. If I do the rounding for each city individually as I would when summing up all trade routes, the lost :commerce: will be wrong, producing incorrect totals.

There's absolutely no way to produce correct display totals.

So, what to do given that I have to display slightly incorrect trade totals? My suggestion is to show the unrounded total to indicate that it's approximate, just as the unrounded value is shown when hovering over a single trade route in a city.
 
If my understanding is correct, the trade routes are in fact added up before they are rounded. Since there are no other other ways for base commerce to get rounded except for Bureacracy capitals, it's only really a big loss when you have 1 trade route at like _.75:commerce:.

I just checked in-game, and can confirm you what Jdog's code says: trade routes modifiers are applied on a per trade route basis, not on the sum. So with a city having 4 trade routes with a base value of 1, adding a harbour does not change anything (except health) since it changes 4*round(1) by 4*round(1.5). It this is not a bug, it is at least counter-intuitive, especially when the trade route panel displays 2 because you have an oversea trade route (base value similar, but additional modifier).

I also just noticed that the to compute their final output in terms of science/gold/... trade routes in a bureaucracy capital are multiplied 3 times: 1 time for things like a harbour, 1 time for bureaucracy, and 1 time for multipliers such a library or market :crazyeye:
 
I think you're all saying the same thing but not realizing it.

  1. In the normal game, trade route yields are rounded per route causing Harbors to be nearly useless.
  2. In BULL with Fractional Trade Routes enabled at compile-time, trade routes are rounded after adding all the routes for a single city.
I wouldn't call this a bug as it works exactly as designed; but it is suboptimal.

Note that while other multiplier buildings such as the Forge get rounded off sometimes, the rounding happens after all multipliers are applied. While a 7 :hammers: city with a Forge gains only +1 :hammers:, adding Organized Religion provides +3 :hammers: in total. If the rounding happened for each multiplier, the total would be +2 :hammers:.

Trade routes are doubly-shafted because you lose to rounding for each route in a city whereas rounding on other values only bites you once per city.
 
I just checked in-game, and can confirm you what Jdog's code says: trade routes modifiers are applied on a per trade route basis, not on the sum. So with a city having 4 trade routes with a base value of 1, adding a harbour does not change anything (except health) since it changes 4*round(1) by 4*round(1.5). It this is not a bug, it is at least counter-intuitive, especially when the trade route panel displays 2 because you have an oversea trade route (base value similar, but additional modifier).

I also just noticed that the to compute their final output in terms of science/gold/... trade routes in a bureaucracy capital are multiplied 3 times: 1 time for things like a harbour, 1 time for bureaucracy, and 1 time for multipliers such a library or market :crazyeye:

Did you read my other post, directly before that one? Sorry I didn't make it clear before but I was talking only about the Fractional Trade Routes component in BULL, as EF repeated also.
 
Did you read my other post, directly before that one? Sorry I didn't make it clear before but I was talking only about the Fractional Trade Routes component in BULL, as EF repeated also.

Sorry if I wasn't clear either. I just wanted to make it clear what is counter-intuitive with trade routes now.

EF > not sure what you wanted to say with the forge/OR example; trade routes also are rounded after all multipliers are applied. Fortunately :) But, as you said, the thing is that you lose commerce for each trade route.

Anyway, yes, it works as designed. I'm just not sure this design is good :( But if the change is not implemented, I'll perhaps just steal PIG's code and compile it for myself ;)
 
Sorry if I wasn't clear either. I just wanted to make it clear what is counter-intuitive with trade routes now.

EF > not sure what you wanted to say with the forge/OR example; trade routes also are rounded after all multipliers are applied. Fortunately :) But, as you said, the thing is that you lose commerce for each trade route.

Anyway, yes, it works as designed. I'm just not sure this design is good :( But if the change is not implemented, I'll perhaps just steal PIG's code and compile it for myself ;)

Hmm, it's not PIG's code but BULL's (search for fractional trade routes). It's not released for PIG yet anyway.
 
My point about the Forge and OR was more to what PoM said that those lose due to rounding as well. I'm saying that trade routes lose more.

Say you build a Harbor that gives you +0.5:commerce: for each 1:commerce: route. If you have 1 route, you lost 0.5:commerce: once. This is akin to losing a hammer that the Forge could give you. If you have two routes, you lost 1:commerce: total, but if you compare it (albeit a bit strained) to having a Forge and OR together, you would lose less from the latter combination because the modifiers add before being rounded.

All I'm saying is that the design is more flawed than just rounding at the city level. Trade routes work as :hammers: would if each individual building/civic's multiplier were rounded individually. In other words, trade routes are doubly-screwed. :p

The only question is whether or not Fractional Trade Routes should be included in the UP. While I think the loss due to rounding of trade routes per route is bad design, I have a hard time considering it a bug that needs patching in the UP.
 
Ah, right, got your analogy now.

Yeah, I know it's not a bug per se. But this is an unofficial patch after all, not just a squash-bug compilation ;) The same way, the paratrooper behaviour has been changed, and I know I would gladly include the change to the cottage/workshop swapping behaviour; but that's not the question, let's not start on this it's only an example :)
 
I have fixed the bug that allows you to acquire unlimited free techs from Oracle/Liberalism in BULL. It involves some changes in CvPlayer.h/cpp and CvDLLWidgetData.cpp and is in the BULL repository. I believe my solution will work in multiplayer games, too.

Didn't the bug also have something extra to do with reloading a game? That sounds familiar but I couldn't find anything related to that.

BTW, has 1.3 been released? I only see up to 1.2 on the first page.
 
EF. I noticed in BULL your new code for CvPlot::getLatitude() that you refer to hard numbers like 60 a few times instead of things like GC.getMapINLINE().getTopLatitude().

Is this going to be a problem? To merge this change in from BULL I'd need to overwrite the function getLatitude that was changed for the UP.
 
Oh, was getLatitude() changed in the UP? I'm merging the latest UP into BULL today but haven't gotten started yet--ACO took forever! :p I'll go check it out now.

The 60 is "number of minutes per degree" and not map-dependent. I use the map's top and bottom latitudes appropriately. I do, however, use -180 and 180 as the full range of longitude values because CvMap doesn't have anything for longitudes. The game doesn't have longitudes in it, so this won't matter. I included them only because it looks cooler than just latitude in the display.
 
[gripe]Ugh, ACO basically lays waste to the CvGameTextMgr... Merging anything else with the CvGameTextMgr after you've got ACO is such a pain... [/gripe]

It's true. :lol: It's the price one pays for lots of detail and options. At least ACO is completely contained in setCombatPlotHelp(), right? It pretty much claims that function for its own.

If you're using winmerge though, it has trouble identifying it as one big change because of all the common little snippets. This definitely makes merging a pain if you already have other code in setCombatPlotHelp().
 
Oh, was getLatitude() changed in the UP? I'm merging the latest UP into BULL today but haven't gotten started yet--ACO took forever! :p I'll go check it out now.

The 60 is "number of minutes per degree" and not map-dependent. I use the map's top and bottom latitudes appropriately. I do, however, use -180 and 180 as the full range of longitude values because CvMap doesn't have anything for longitudes. The game doesn't have longitudes in it, so this won't matter. I included them only because it looks cooler than just latitude in the display.

Oh, right. My mistake.

For now I've replaced the UP code with yours - I can't see any problem yet with doing that.
 
I reworked the lat/long functions a little. They now detect when the range should go to the full extend (-90 and 90) based on wrapping. I had to touch the same three files. Note that I renamed the getReal___Minutes() functions by removing the Real part since it's unnecessary since they have Minutes on the end.

BTW, the time sink of merging ACO for me involved converting to using getBugOptionXXX() instead of getDefineXXX() and creating all the options (there were far more options this time around) in BUG. This meant I had to write the XML, comie up with hover text, and place them on the options screen. I also had to extend BUG's option core a little to make it so I could use the same list of choices for the 13 similar view options so the translators wouldn't be annoyed. ;)

Yes, the code is long and formatted in a . . . let's say creative manner (though better this time), it really shouldn't take other modders much time at all to merge since they can do a straight copy-paste of the entire function.
 
Top Bottom