Thalassicus / cep-bnw

Civ V Communitas Expansion Pack
32 stars 22 forks source link

Policy Bugs #54

Closed AnastaseAlex closed 10 years ago

AnastaseAlex commented 10 years ago

(edited for clarity - moved fixed bugs here)

See: http://forums.civfanatics.com/showthread.php?t=516218 & http://forums.civfanatics.com/showthread.php?t=517797

stackpoint commented 10 years ago

Fine arts was a text error. The intended behavior was 1 culture per 2 happiness.

Cultural centers and ceremonial rites was a syntax error.

See: https://github.com/Thalassicus/cep-bnw/commit/01518c22326f7c5a2650352e6d825a911268e525

stackpoint commented 10 years ago

POLICY_UNITY doesn't seem to have any code for implementation.

RevealMinorCapitals for POLICY_PATRONAGE doesn't seem to be implemented.

POLICY_CULTURAL_CENTERS does not have any code for the 33% culture modifier.

stackpoint commented 10 years ago

Ceremonial Rites and Republic bugs was due to the Great Hall having greater city defense and culture flavors than the normally available buildings.

Grant has lowered the values here: https://github.com/Thalassicus/cep-bnw/commit/563233e640e1965be7a463cd0481bf00ba786dac

skodkim commented 10 years ago

Version 3.10: TXT_KEY_POLICY_FLOURISHING_OF_ARTS_HELP says "[ICON_CULTURE] Culture: +1 per Village"

As far as I can tell from the files this effect is not implemented. Instead is gives production boosts for UNITCLASS_ARCHAEOLOGIST (and maybe other things as well).

Hawawaa commented 10 years ago

In the honor tree discipline doesn't give you a free great general.

http://forums.civfanatics.com/showthread.php?p=12957100#post12957100 files here.

Hawawaa commented 10 years ago

Also it doesn't give +1 movement to great generals.

GrantSP commented 10 years ago

The free Great General should now be fixed, it simply lacked the definition. However, the free promotion is another matter. There is a new table defined for giving free promotions to units based on the policy chosen, it is at the moment lacking any code to give it functionality. The promotion Mobility 1 is given to Great Generals after choosing Discipline, at least it is coded that way.

GrantSP commented 10 years ago

Regarding the Wealth opener. I have successfully coded it to give 15% to Gold & Production but there are a couple of questions about it that need clarifying.

1) The tooltip in the CityView shows 15% from Misc & Policy Modifier: 15%. It doesn't appear to be duplicated in the total yields, just a visual thing, but it is distracting. 2) Protectionism, which is also in the Wealth tree, also gives a modified yield to Gold & Production of 10%. This means you can with 2 policy picks get a 25% yield boost. Add to that the +2 to Production from coastal cities from Maritime Infrastructure, also in Wealth, and your Production is rapidly increased with 3 policy picks. 3) The wonder unlock of Panama Canal needs the TECH_DYNAMITE, in the Industrial era, to build yet this policy is taken in the Medieval era, perhaps a change in TECH for this wonder or another more easily gained wonder to replace it.

skodkim commented 10 years ago

@GrantSP

I just noticed that the Wealth fix you just committed says 15% boost for Gold and Production in Ceg/Ceg/Policies/Trees/CEP_Wealth.xml but 10% in Ceg/Ceg/Text/en_US/Policies.sql

\Skodkim

GrantSP commented 10 years ago

10% boost is for the Protectionism policy inside the Wealth tree. I added the 15% to the the Wealth opener but added no text into the mod.

Thalassicus commented 10 years ago

Patronage should be fixed on line 812 of Cat_Events.lua: fcb3e5e36b752d386686c1989228acf3bedd5fcc

skodkim commented 10 years ago

@GrantSP

Sorry!

\Skodkim

skodkim commented 10 years ago

With 3.10.2 (+hotfix) I've found that Ceremonial Rites do not give +1 culture to monuments.

The code seems to be omplemented in Policies\CEP_End.sql but are you sure the underlying code Works? As I recall there was a problem related to giving culture to Wonders from a policy (this is probably the same code) due to YieldLibrary.lua not being fully updated.

Youl'll probably need to test it ingame as there's nothing in the logs.

\Skodkim

Hawawaa commented 10 years ago

still missing +1 movement for generals 3.11 http://forums.civfanatics.com/showthread.php?p=12970695#post12970695

GrantSP commented 10 years ago

I'm not sure the Ceremonial Rites free buildings based on era is good idea. If you beeline your research and reach Drama early, you have the weird setup of Theatres in your new city but still having to build a Monument. Admittedly there is no conflict with one requiring the other, it is just odd in a gameplay sense. Much better I think to make it just the first tier building, the Monument.

GrantSP commented 10 years ago

We should change the text for the policies that grant free buildings to mention the need for researching the required tech.

GrantSP commented 10 years ago

The policies that give free building types don't give the same result. Ceremonial Rites gives Culture buildings based on era yet Republic, which looks to use similar code doesn't, it only gives Walls as the defensive structures.

Also as mentioned with the "Free" culture buildings, these walls are not free as in you still pay maintenance.

Is the description in error? Are they free only in the initial "purchase" but not in maintenance? If so this needs changing to provide truly FREE buildings.

skodkim commented 10 years ago

@AnastaseAlex

Did you see my post regarding extra culture on culture Buildings (You haven't reopened it in post 1)?

\Skdkim

AnastaseAlex commented 10 years ago

Just did :)

skodkim commented 10 years ago

Sorry for bumping this but is anyone assigned to the bugs?

Its just that policies was supposed to be 3.10 and there doesnt really seem to be much going on concerning these.

Have been looking at some of them myself but it seems it is beyond my simple xml modding abilities :( maybe we need Thal to go through some of the lua code behind it..?

\Skodkim

Thalassicus commented 10 years ago

I fixed a few of these tonight. I'll look at more later when I can.

skodkim commented 10 years ago

Just did a Little test of the remaining using 3.12. Most descriptions are ok, but heres som extra info on two of the described bugs:

-Ceremonial Rites: Just tested the free Buildings from policies in Vanilla and there it works as intended - the given Buildings are Maintenance free -Honor opening in fact makes the Temple of Artemis available but I'm pretty sure it's only the turn after it's been picked (probably the same for alle policy unlocked wonders then...)

civ5screen0016 civ5screen0017

\Skodkim

Thalassicus commented 10 years ago

That's interesting, so it appears the "RevealMap" effect actually does the same thing as the reveal map Ancient Ruins reward, centered on the capital. I expected it to do the full map reveal of the Satellites technology. I'll have to code it manually.

skodkim commented 10 years ago

Just noticed one small thing: The description of the Tradition Opener shoud say +3 Culture in Capital in stead of just +3 culture

\Skodkim

stackpoint commented 10 years ago

There shouldn't be anything delaying the unlock of wonders in the openers since that's all done with vanilla code.

There is no way to grant maintenance free buildings using lua code and the code the vanilla policy uses is locked in the DLL. This is why buildings are granted with their maintenance cost. I guess the question is it's necessary for the buildings to be maintenance free.

skodkim commented 10 years ago

Are you sure about the maintenance free buildings being impossible with lua? I was certain it worked in GEM.

Anyway I just thought of a little workaround for the two Ceremonial Rights bugs. Best part is that they are both based on already working code.

You can get maintenance free buildings from wonders. We can make the policy create a dummy building (invisible in civilopedia and city view, non-buildable otherwise, etc…) in the first four cities and make this buildings do only one thing: Give a free monument. One drawback though is that it can only create one type of free buildings, e.g. you can’t make it give a theatre instead.

I’m pretty confident you can also make a wonders grant additional yields to certain building classes. I’m pretty certain I’m using this code right now to add 1 science to libraries. This means that we can create a dummy building (possibly the one from above – the effects probably won’t stack even though we build four) that adds 1 culture to monuments, theatres, etc…

Furthermore it's mostly copy-paste so it would only take me ~15 minutes to make and test. Unfortunately I’m off to work right now (with this in the back of my head).

EDIT: Just brainstorming here: You can add buildings from policies based on flavour and the one with the highest flavour will be added if its available in discovered techs, right? Maybe we could add different dummy buildings, that add different culture buildings. The thing I'm unsure of right now is whether we can control a buildings flavour manually as we'd need to make sure that the dummy building that grants free theatres would have to have higher flavour than the one that ads free monuments (etc.). This is normally an automatic calculation based on yields (and we can't let the dummy buildings add yields), right? Does it also involve free buildings, i.e. will the Great Library have a higher flavour for techs because it grants a free library than if it didn't?

\Skodkim

skodkim commented 10 years ago

Well I kind of got halfway. I got the Buildings working, but I'm having some problems with getting them to Work with the policy only a Little help needed:

Ad1 (The free cultural buildings): Seperate dummy Buildings add BUILDINGCLASS_MONUMENT, BUILDINGCLASS_AMPPHITHEATRE, ... (only these two made now as this was a test). Tested using the fire tuner etc. to give myself these dummy Buildings and they give a free cultural Building. Problem: The dummy Buildings are automatically assigned a flavor based on the cultural Building they add, but to make sure the policy selects the dummy Building rather than the maintenance free cultural building they're supposed to add I need to give them a high flavor for culture. I tried tweaking CEAI_Buildings.sql to do this but I can't get it to work. Can anyone help?

Ad 2 (the bonus culture from cultural buildings). Orinially Created a dummy building that adds this and it works. However it turns out that there's a standard function (Policy_BuildingClassCultureChanges) that adds culture to buildingclasses when a policy is picked. It Works!

I've attached an edited CEP_Tradition.xml with the new Building (still test phase - assigned to agriculture to be make flavours viewable for me in tech tree and they show up in civilopedia) and with the +culture for cultural Buildings fix. Also attached a version of CEAI_Buildings.sql where I tried to increase the dummy buildings flavour for culture. If anyone can help with this I think I can wrap it up!

ceai_buildings sql cep_tradition xml

\Skodkim

stackpoint commented 10 years ago
stackpoint commented 10 years ago

@skodkim you shouldn't need to create dummy buildings for the missing additional culture from those policies. The yield library should be fixed instead.

Using dummy buildings to grant FREE buildings is interesting but may have some problems.

First, you'd have to tie those dummy buildings into the same technology that the cultural buildings are tied to in order to make sure that it won't unlock early. This may be doable using the IsVisible tag but I don't know if that would hide the building in the TechTree, additional code may be required.

Second, you may have been encountering code Thal has been using to delete flavors from dummy buildings in the CEAI_Buildings.sql:664 below:

-- Dummy buildings automatically assigned to cities
DELETE FROM Building_Flavors WHERE BuildingType IN (SELECT Type FROM Buildings WHERE Cost = 0 OR Cost = -1);

Which may be why you were not getting the flavors you desired on your buildings. You may want to assign flavor values to CEP_End.sql instead as that will run after the dummy building flavors are zero'd out.

Finally, I think @Thalassicus is thinking about removing that policy altogether if I remember correctly. You should wait for more input from him before continuing further on this. Good work figuring out a workaround to the FREE problem though.

skodkim commented 10 years ago

Did you fix it then? It looks like it based on the list. If so I can hardly wait for a new release.

The Building is hidden in the tech tree as well as city view and civilopedia (99% sure I tried this).

Concerning flavors I did try setting cost=1 to avoid it being "caught" by the very line you mention but it didn't Work.

\Skodkim

stackpoint commented 10 years ago

I haven't fixed it, the bug should still be listed in the top post.

If you set the 1 and the flavors are still missing, you may want to set a high culture value (100+) manually or try commenting out the deletion code.

The Policy_FreeBuildingFlavor code also checks whether or not the building is constructable in that city. But if you make a dummy building constructable then won't the AI be able to build it in their cities? I believe the IsVisible tag only effects human players.

EDIT: Please ignore my (deleted) post below. I was working off an incorrect database with the incorrect flavors.

skodkim commented 10 years ago

Will have a look later. At least the code fit adding culture to buildings is default bnw stuff and easy to add.

\Skodkim

stackpointer notifications@github.com skrev:

Also, I just reviewed the code that grants the building:

function PlaceBuildingOfFlavor(city, flavorType)
  ...
  local buildingID, buildingFlavor =
Game.GetMaximum(City_GetBuildingsOfFlavor(city, flavorType))
  ...
end

And it seems like City_GetBuildingsOfFlavor() checks if the city can build the building using City_CanBuild. The GetMaximum() function seems to be flawed since all the culture buildings have the same value (8) and the function tiebreaks with a coinflip:

------------------------------------------------------------------
-- Game.GetMaximum(table) returns maximum item from a list
--
function Game.GetMaximum(list)
  local maxIndex = -1
  local maxValue = -math.huge
  for index, value in pairs(list) do
      if value > maxValue then
          maxIndex = index
          maxValue = value
      elseif value == maxValue and 1 == Map.Rand(2, "Game.GetMaximum") then
          maxIndex = index
          maxValue = value
      end
  end
  return maxIndex, maxValue
end

Reply to this email directly or view it on GitHub: https://github.com/Thalassicus/cep-bnw/issues/54#issuecomment-32116554

Sendt fra min Android telefon med K-9 Mail. Undskyld hvis jeg er lidt kortfattet.

skodkim commented 10 years ago

Just tried deleting the last two delete statements of CEAI_Buildings.sql, also tried setting flavor manually and raising the flavor of a factor 20 in the sql but nothing seems to Works.

You can prevent the AI from building the Building by setting Cost=-1, like BUILDING_AMSTERDAM_BOURSE (right?)

Why do both monuments and Theatres (amphitheatre) have a culture flavor of 8 when the latter has a higher culture yield than the former?

Anyway the +1 culture/Building Works and I'll make a commit later.

\Skodkim

stackpoint commented 10 years ago

I was working off the incorrect database as noted in my edit here: https://github.com/Thalassicus/cep-bnw/issues/54#issuecomment-32116409

So the monument and theaters don't actually have the same flavor values. Also in my edited post: "The Policy_FreeBuildingFlavor code also checks whether or not the building is constructable in that city." So if you set the cost to -1, I don't think it'll be constructable in that city.

skodkim commented 10 years ago

Actually what I was saying was that I'm seeing monuments and theatres with the same flavour values for culture (plus a couple of others for theatres).

\Skodkim

stackpoint commented 10 years ago

Actually it seems like I was twice wrong. The flavor values for all those buildings in that line is 8. This normally would not be a problem but now that the buildings are all delinked, the CanBuild() function will take in account all of them. This will cause the building chosen by GetMaximum() to be chosen by coinflip when there are tied flavor values.......

skodkim commented 10 years ago

Yeah. I wrote a pm to Thal at civfanatics mentioning this problem. Hopefully he'll return with a solution soon.

\Skodkim

stackpointer notifications@github.com skrev:

Actually it seems like I was twice wrong. The flavor values for all those buildings in that line is 8. This normally would not be a problem but now that the buildings are all delinked, the CanBuild() function will take in account all of them. This will cause the building chosen by GetMaximum() to be chosen by coinflip when there are tied flavor values.......


Reply to this email directly or view it on GitHub: https://github.com/Thalassicus/cep-bnw/issues/54#issuecomment-32190538

Sendt fra min Android telefon med K-9 Mail. Undskyld hvis jeg er lidt kortfattet.

stackpoint commented 10 years ago

Exploration: Pioneer spirit gave an archer and Chang Suek (not defensive unit) when playing with Siam.

It looks like Archers have high enough FLAVOR_DEFENSE values to be considered defensive units. This wasn't a problem in GEM when we had a vanguard line but now that we don't, various units may be considered defensive units depending on their flavor values. I'll consider that issue closed unless something better is suggested.

stackpoint commented 10 years ago

Consulates: says that it gives a +20 to the resting influence of city states, which it does, but it also gives +20 instant influence with city states, which I don't think is intended.

Cannot replicate this bug.

AnastaseAlex commented 10 years ago

Pioneer Spirit: units: It is just the disparity of the units given that is an issue: an archer is significantly weaker than a knight for example. I guess it depends on what technologies you have at the time, but instead of writing defense units, the description could be changed to two military units.

AnastaseAlex commented 10 years ago

Consulates: I thought that had gotten fixed a month ago...

stackpoint commented 10 years ago

@AnastaseAlex, consulates may have been fixed a month ago, I was away around that time so I couldn't say.

GrantSP commented 10 years ago

I could be wrong here but I don't believe there are ANY changes made to the Consulates policy, other than a text change. The exact vanilla text is not clear to me at the moment but I am pretty sure the new text says much the same but in a different way. Probably don't need to have a change for this if we haven't really changed the policy.

I just looked through the source on the web, painfully slow compared to working on my own copies, and it looks like the only changes are to assign FLAVOR to this policy and the aforementioned text change.

If this is the case then there can be NO mod bug for this that needs fixing.

AnastaseAlex commented 10 years ago

I tested the consulates again today and indeed it did not give an instant +20 influence. I will remove the bug.

skodkim commented 10 years ago

Don't know if this should go in the policy updates thread https://github.com/Thalassicus/cep-bnw/issues/167 but I just tried Republic with 3.14 and got no defense Buildings. Looking at the code it seems to be implemented but there may be other leftovers (unwanted effects) from the past. Will see if I can figure it out when I have more time.

\Skodkim

GrantSP commented 10 years ago

Are you sure you have Masonry?

skodkim commented 10 years ago

D... Feel like such a fool now. I diddn't have masonry (wasn't even aware that walls required masonry though). I was pretty sure that I had sen this in an ongoing game (where I was way leter in techs) so I wanted to make a quick test without other mods (just IGE). That'll teach me not to make quick tests at 5 am before the rest of the Family wakes up and Work Begins :smile:

Think the part about redundant code can be innpred too :blush:

\Skodkim

stackpoint commented 10 years ago
BuildingType                FlavorType              Flavor
BUILDING_AIRPORT            FLAVOR_CITY_DEFENSE     16
BUILDING_ALHAMBRA           FLAVOR_CITY_DEFENSE     8
BUILDING_ARSENAL            FLAVOR_CITY_DEFENSE     16
BUILDING_CASTLE             FLAVOR_CITY_DEFENSE     16
BUILDING_HIMEJI_CASTLE      FLAVOR_CITY_DEFENSE     8
BUILDING_MILITARY_BASE      FLAVOR_CITY_DEFENSE     16
BUILDING_MUGHAL_FORT        FLAVOR_CITY_DEFENSE     32
BUILDING_NEUSCHWANSTEIN     FLAVOR_CITY_DEFENSE     32
BUILDING_WALLS              FLAVOR_CITY_DEFENSE     16
BUILDING_WALLS_OF_BABYLON   FLAVOR_CITY_DEFENSE     32

Here are the buildings with FLAVOR_CITY_DEFENSE, looking at this list it seems possible that you were being granted airports instead of the more conventional buildings.

skodkim commented 10 years ago

Way later in techs was propably a bad way to phrase it - I think I was researching Civil Service and I know I had Masonry because I had built the Pyramids long ago (changed prereqtech to Masonry in local modmod - trapping just seems wring to me). I'll keep an eye out if I get to pick it again.

\Skodkim

stackpoint commented 10 years ago

It's also possible that one of the other mods you play with adds another building with a FLAVOR_CITY_DEFENSE value. You should check your Civ5DebugDatabase.db file.