Thalassicus / cep-bnw

Civ V Communitas Expansion Pack
32 stars 22 forks source link

Royal Library #208

Closed GrantSP closed 10 years ago

GrantSP commented 10 years ago

The tooltip for this building is incorrectly setup to show the XP gained by Great Works in it.

At the moment it shows only 1 XP per Great Work. I have made some changes to the code and managed to get it show the correct 10 XP.

Code:

elseif lineType == "ExperiencePerGreatWork" then
  for row in GameInfo.Building_DomainFreeExperiencePerGreatWork{BuildingType = objectInfo.Type} do
   lineValue = row.Experience
   InsertBuildingSubStat()
  end

I copied the ExperienceDomain as a template. But the result is not what I expect. This function shows 3 lines, 1 for each domain.

Experience: 10 per Great Work Experience: 10 per Great Work Experience: 10 per Great Work

Not exactly what we need. (As I am typing this I am also checking the code and it looks like the XP gained for each domain using the table Building_DomainFreeExperiences and appears that ALL the buildings that use this table don't have it shown in the tooltip/pedia.)

I can see that the TXT_KEYs are different. The barracks type buildings use the TXT_KEY_BUILDING_EFFECT_EXPERIENCE.

[ICON_WAR] Experience: {3_value} for all Units

The Royal Libray uses: TXT_KEY_BUILDING_EFFECT_EXPERIENCE_PER_GREAT_WORK

Experience: {3_value} per Great Work

How do I get only 1 line to show instead of 3 ?

GrantSP commented 10 years ago

I guess there may be 2 ways to do this.

1) Make two more TXT_KEYs so there is one for each domain and then the Royal Library will have the three lines shown but it would indicate the domains that get the benefit.

2) Change the function somehow to not look for each domain listed in the table but just once for each building type.

I guess the problem would then be it may be possible in the future to have a building that gives different values for different domains.

stackpoint commented 10 years ago

You can specify the domains like the ExperienceDomain uses:

    elseif lineType == "ExperienceDomain" then
        for row in GameInfo.Building_DomainFreeExperiences{BuildingType = objectInfo.Type} do
            lineValue = row.Experience
            lineExtra = Locale.ConvertTextKey(GameInfo.Domains[row.DomainType].Description or GameInfo.Domains[row.DomainType].Type)
            InsertBuildingSubStat()
        end
GrantSP commented 10 years ago

I had that at first but removed it in the hope it would not specify the different domains and thus just give one value.

stackpoint commented 10 years ago

If you assume that all the entries in Building_DomainFreeExperiencePerGreatWork will have all three domains specified then you can use:

  for row in GameInfo.Building_DomainFreeExperiencePerGreatWork{BuildingType = objectInfo.Type AND DomainType = "DOMAIN_LAND"} do
GrantSP commented 10 years ago

Thanks, will try it later. Do you think the text should state which domains get the bonus?

I'm a bit between ideas. We should show as much info as needed but it may look strange to have a landlocked city give a boost to naval units.

stackpoint commented 10 years ago

If we ever plan and having this table give bonuses that don't include all three domains then we should specify it. But if we only use it for the Royal Library then there's no need.

GrantSP commented 10 years ago

Adding in AND DomainType = "DOMAIN_LAND" to the code renders the entire lua code useless. ;(

Will have to keep experimenting.

GrantSP commented 10 years ago

This is becoming tricky to find a solution because every time I try and look to another piece of code as an example the end results are different despite the underlying code indicating it should be the same.

Have a look at 3 examples, Hagia Sophia, Statue of Liberty and Jade Hall. All these wonders use the table, Building_SpecialistYieldChanges which gives a value based on different types of Specialists (this is the sort of thing I was after as the tooltip shows just 1 value for ALL the types). hagia sophia statue-of-liberty jade hall

Now they all should be displaying the tooltip in the style defined by: TXT_KEY_BUILDING_EFFECT_YIELD_FROM_SPECIALISTS which is: {1_prefix}: {3_value} for {4_extra} {TXT_KEY_IN_ALL_CITIES}

Clearly they don't. Why? If I knew that I could probably figure out the Royal Library. The lua is clear in that whichever effect is used, based on the table, a predefined text entry is used to display the information. _TWInit.lua lines 297 - 309.

stackpoint commented 10 years ago

They do? For example, in your last screenshot:

Science: 1 for Unemployed Citizen, Artist, Scientist, Merchant, Engineer in all Cities
{1_prefix}: {3_value} for {4_extra} {TXT_KEY_IN_ALL_CITIES}
1_prefix = Science
3_value = 1
4_extra = Unemployed Citizen, Artist, Scientist, Merchant, Engineer
TXT_KEY_IN_ALL_CITIES = in all Cities
stackpoint commented 10 years ago

Adding an IF check would be just as effective as an AND in that bracket check (I don't know what the correct syntax for that code is):

    elseif lineType == "ExperiencePerGreatWork" then
      for row in GameInfo.Building_DomainFreeExperiencePerGreatWork{BuildingType = objectInfo.Type} do
       if(row.DomainType == "DOMAIN_LAND") then
        lineValue = row.Experience
        InsertBuildingSubStat()
       end
      end
GrantSP commented 10 years ago

I knew you were going to say that. There is however a significant difference in the formats. Statue of Liberty makes a bonus to Production for ALL specialist types yet the text only reads "SpecialistsMusician, Writer" Hagia Sophia just uses the generic term "Specialist" and Jade Hall defines them individually.

I know the 'essence' of what they are saying is the same but the text I would have thought should be consistent.

PS. You posted your next post before I could finish, so this applies to the one before :smiley:

GrantSP commented 10 years ago

So the inclusion of a single "if" should do the trick?

Nope. Placing an "if" there makes the code break.

Ahh, I may have missed placing the "End". Trying again.

stackpoint commented 10 years ago

If they aren't consistent then you should take a look at how the lineExtra variable is built since that is what those sections are.

GrantSP commented 10 years ago

Ok tried again with the "end" break and the code still is broken. We can't seem to place "if/end" breaks here.

Regarding the lineExtra variable, in discussion with @Thalassicus last week about these sort of things he said the actual naming of the variables is irrelevant it is only the position in the function that is important. So in this case {4_extra} could just as well be called {4_greencheese} or just {4}. In fact he has changed the spreadsheets to render all the English variables to be just {numerals} in all the language columns.

So since the function that defines this text is the same for all three wonders the fourth variable is also the same and I would have imagined it would give the same result. But apparently something else happens.

If I can see where that fourth variable pulls the info and formats it, and make heads or tails of it. Perhaps I can figure this out.

I'm pretty sure the function in question is found at lines 542 - 569

GrantSP commented 10 years ago

@Thalassicus has added in some very helpful info on the function GetYieldInfo, if we had similar for the other functions then maybe I could make sense a bit more.

stackpoint commented 10 years ago

Can you paste your code?

GrantSP commented 10 years ago

Opening post.

stackpoint commented 10 years ago

With my IF conditional with the error the lua log produces.

GrantSP commented 10 years ago

I stuffed up. Placed the "end" in the wrong place. Copied your code from your comment and it works fine. Sorry. Ok, that fixes the Royal Library, sort of. It is a bit of a hack though in that it isn't actually telling the whole story, but for the time being it gets us by.

We can close this now unless you want to leave it open to figure out more of the goings on with the tooltip code?

GrantSP commented 10 years ago

Looking through that code, there appear to be elseif & if blocks without ends.

The GreatWorkSlotType block and the SpecialistType block don't look to have matching ends to their ifs.

What am I not seeing?

Nevermind. I see what it is.

stackpoint commented 10 years ago

The formatting of LUA IF trees are:

if conditional then
   code
elseif conditional2 then
   code
else
   code
end
GrantSP commented 10 years ago

Yeah. Saw that in the code. I am now brushing up on my lua reference manual.

stackpoint commented 10 years ago

Regarding Building_SpecialistYieldChanges, it seems like the table has some incorrect entries. The Statue of Liberty has duplicate entries and the Jade Hall is missing the Writer and Musician Specialists which are causing the display issues.

You are correct that Building_SpecialistYieldChanges does use special code that detects when all specialists are used and turns it into "all specialists" but I don't know how to replicate that code for our use in domains.

GrantSP commented 10 years ago

So it does. In all the time I spent looking at that table and I never once saw those duplications. There is a small piece of code that looks like it adds all the Specialists and if they are all listed it changes the text to show "Specialists".

MAX_SPECIALISTS = 0
for specialistInfo in GameInfo.Specialists() do
MAX_SPECIALISTS = MAX_SPECIALISTS + 1
end

This is at the top of the file. In the actual function that handles this table the last if/end block is:

if   yieldNum[row.YieldType][row.Yield] == MAX_SPECIALISTS then
            yieldList[row.YieldType][row.Yield] = Locale.ConvertTextKey("TXT_KEY_PEOPLE_SECTION_1")
end

That is a vanilla TXT_KEY that simple says: "Specialists". That's why the Statue of Liberty screenshot is mangled. It shows the text for ALL the specialists and then the extra two get tacked poorly onto the end of the {4_extra} variable.

So, if we could make a code block that counts the domains and if they are listed it changes the text to call a different TXT_KEY that is the same as the vanilla: "all Units". Then this table could be used by other buildings and the bonus could be applied to just one domain and it would show as such.

GrantSP commented 10 years ago

As an aside I have made those changes to the Jade Hall & Statue of Liberty here & here.

stackpoint commented 10 years ago

The function ConvertYieldList probably has something to do with how the tooltips handles MAX_RESOURCES and MAX_SPECIALISTS but I can't decipher it at all. So that'll probably be the end of my participation unless you need help with anything else.

GrantSP commented 10 years ago

Thanks. All done here.

GrantSP commented 10 years ago

Ok one last thing. Now that I have made the changes to the Jade Hall & Statue of Liberty all three of these wonders now provide a boost to their yield with ALL the specialist types. I was imagining that the text would now look like the Hagia Sophia as it was the only unchanged building. Meaning it would say: Yield :1 for all Specialists Nope. Now it lists all the specialists separately. Perhaps that MAX_SPECIALISTS code isn't quite doing what I thought. Ah well. Still looks much better than before though.