Thalassicus / cep-bnw

Civ V Communitas Expansion Pack
32 stars 22 forks source link

YieldLibrary Activation #179

Open stackpoint opened 10 years ago

stackpoint commented 10 years ago

I'm going to start slowly reactivating YieldLibrary functions in hopes of restoring functionality to some CEP features.

There are also a lot of yield bugs that need to be confirmed fixed. Any outstanding issues should be brought up here so they can be troubleshooted and collected here.

GrantSP commented 10 years ago

In YieldLibrary there is this block:

            else
            local unitID        = city:GetProductionUnit()
            local buildingID    = city:GetProductionBuilding()
            local projectID     = city:GetProductionProject()
            if unitID and unitID ~= -1 then
                return City_GetBaseYieldRateModifier(city, yieldID, GameInfo.Units, unitID)
            elseif buildingID and buildingID ~= -1 then
                return City_GetBaseYieldRateModifier(city, yieldID, GameInfo.Buildings, buildingID)
            elseif projectID and projectID ~= -1 then
                return City_GetBaseYieldRateModifier(city, yieldID, GameInfo.Projects, projectID)
            end
        end

Can we add new elseif clauses here to account for modifiers from Belief or Policy tables?

stackpoint commented 10 years ago

Right, but there's nothing wrong with the function City_GetBaseYieldRateModifier. It's all in how the tooltip partitions the modifiers. I ended up lumping everything together since I was having trouble understanding the tooltip code and where everything was and needed the percentages displayed as I was focusing on testing the reactivated YieldLibrary function.

It should already be possible to partition the percentages up but I've been having trouble locating and understanding all the tooltip code while I've been focusing on mostly functional changes.

stackpoint commented 10 years ago

Regarding your the second post that went up at the same time as mine, that section of code recursively assigns the itemTable parameter if for some reason it wasn't passed to the function in the first place. I don't believe it has anything to do with the tooltip code.

GrantSP commented 10 years ago

@stackpoint _CityGetBaseYieldRate looks to handle many different yields by calling additional functions geared specifically towards each defined type.

Can we add a function to handle Yields from Belief or other areas?

My other comment wasn't specifically about the tooltip code, just looking at seeing if the YieldLibrary can be made better/clearer/easier.

If the same block of code can be used in many ways it would make it more streamlined. Wouldn't it?

stackpoint commented 10 years ago

City_GetBaseYieldRate already has all the components of all the modifiers since it sums the total modifiers for the city. So it's just a matter of partitioning the components into it's individual functions (some of which have already been done).

GrantSP commented 10 years ago

Maybe we could get Thal to comment his code? Try to make it easier to read.

stackpoint commented 10 years ago

The code is already there as I've stated:

    yieldMod = yieldMod + player:GetGoldenAgeYieldModifier(yieldID)
    yieldMod = yieldMod + City_GetBaseYieldModFromTraits(city, yieldID)
    yieldMod = yieldMod + City_GetBaseYieldModFromPuppet(city, yieldID)
    yieldMod = yieldMod + City_GetBaseYieldModifierFromPolicies(city, yieldID, itemTable, itemID, queueNum)
    yieldMod = yieldMod + City_GetBaseYieldModifierFromGlobalBuildings(cityOwner, yieldID)
    yieldMod = yieldMod + City_GetBaseYieldModFromBuildings(city, yieldID)

I won't have trouble finding the yield modifier code. I was having more problems with identifying the correct section of tooltip code to modify, especially earlier when I wasn't sure the YieldLibrary was granting the correct yields in the first place.

GrantSP commented 10 years ago

I'm not trying to tell you what to do, I'm trying to understand it for myself. I just use the stuff you're on so you don't have to shift your attention too much.

GrantSP commented 10 years ago

The OP has TopPanel.lua as having activated YieldLibrary support, I would question this in light of the discussion in CivFanatics regarding World Congress. The last comment I made there made mention of an apparent discrepancy in the yields shown. Though I may not be seeing the full picture there, perhaps there are some modifiers at play that aren't obvious.

You still have ProductionPopup.lua as needing work, I concur it is desperately in need of a fix. This is the same panel that shows on both the large right-hand side notification and when inside the CityView you choose either Purchase or Production? Specifically the list of yields at the top? Gold in particular is way out. In one saved game the capital showed either 82 or 55 depending on where you look. Next worst yield was the Food which was on average 20% out.

There isn't a straight correlation either, it isn't a case of the yields from one file are less than the other, it is the the yields themselves vary up or down based on which city. I am imagining this is better as that seems to indicate the modifiers are not synced in the files.

Before you do start on those files though, I am trying to sync up the display of them to have them be as consistent as possible. For instance Tourism isn't shown in the list of yields in the ProductionPopup, I'm seeing if I can figure this out before you get to it.

stackpoint commented 10 years ago

The TopPanel.lua has activate YieldLibrary support. It's just a matter of making sure all the math checks out in the YieldLibrary and ToolTip files.

stackpoint commented 10 years ago

The ProductionPopup is this: http://img839.imageshack.us/img839/3581/mcsfood.jpg

We currently don't have ProductionPanel.lua active in our mod so there's no support for it at all. And there's no point in modifying the file until we actually get all the yields and tooltips working correctly in the CityView.lua file in the first place.

stackpoint commented 10 years ago

It seems like I had mistaken ProductionPopup for ProductionPanel. Anyway, I just took a quick look and it seems to be running all the vanilla code for the yield part of the popup so it doesn't have any YieldLibrary support at all. And there's no point in modifying the file until we actually get all the yields and tooltips working correctly in the CityView.lua file in the first place.

stackpoint commented 10 years ago

And isn't Tourism not included in the ProductionPopup in vanilla?

GrantSP commented 10 years ago

Putting aside your double negative in the question, :smiley: nope. It has all the yields except that. I saw somewhere some code that does what I am describing, I couldn't find it yesterday, but I know what I am after and it works, at least it did once.

Also I think your image shows more of the tooltip that occurs over the ProductionPopup, I believe but could be wrong, what is shown on that is determined by the InfoTooltipInclude.lua.

GrantSP commented 10 years ago

This is not a criticism of the work thus far, just an observation. It looks like for every step forward that solves a problem a new one emerges to take it's place. It could almost be considered counter-productive to maintain the code-base as it is with so many undocumented/unclear functions. If it were cut back to the basic minimum and then new functions re-established, or re-written, as they become needed.

stackpoint commented 10 years ago

If you could list all the basic minimum functions of the yieldlibrary then it wouldn't be a huge problem to do this.

GrantSP commented 10 years ago

I barely understand the basics to make an informed list but I will say this. On the wiki, @Thalassicus says this:

"I redesigned the yield functions so they are easier to use. For example, a single function player:GetYieldStored returns stored gold, science, culture, or so on. This is vastly simpler than the disorganized collection of functions required in the past."

On that function I don't understand why Production is not a yield accessed by it, Science, Food, Gold, Happiness, Faith and even an unused CS_MILITARY yield are, but not Production.

Speaking as an absolute novice, any time a yield is needed to be accessed by any vanilla function or modded function, the same values should be used.

So if we use Production as the reference, the displays in TopPanel, CityView, ProductionPopup, WorldView or anything else should be using the same value. Further to that any modifier should likewise work on the same value. Percentage boosts or penalties from Beliefs, Policies, Wonders, CS alliances, Trade, Traits, Un/Happiness, Golden Ages, etc. should also being applied to the same value.

Imagine a city that is making 10Production, 4 from Buildings and 6 from Terrain. If the CityView applies a modifier to the total yield then every other instance the totals are used it needs to be the same. If we have an instance where a belief, for instance, modifies the production and it is shown on the TopPanel but not in the CityView, users have the right to say: "hey what is the true value?"

Looking at YieldLibrary.lua, I feel for you. The task is not an easy one. There are functions to provide base yields, consumed yields, surplus yields, total yields, etc. assuming they are all working together then I can see it is fine, but if just one of these is referencing the incorrect value from an out-of-date function. Well, I don't know.

stackpoint commented 10 years ago

PlayerClass.GetYieldStored refers to national yields and production is a city and unit/building specific yield. There is no global yield stored for production for players.

GrantSP commented 10 years ago

Like I said, 'novice'. :smiley:

stackpoint commented 10 years ago

And I don't really understand what you're trying to say with the rest.

GrantSP commented 10 years ago

Well, that's not surprising. All I'm saying is, I don't know why the yields are so wrong. I can't believe it was like this before BNW. Nothing seems to match. Depending on where you look, the yields can vary dramatically. If the yield says X in one place it should say X in the other places. The totals and the breakdowns of each don't always match. Look at the World Congress post in the forum to see what I mean.

stackpoint commented 10 years ago

This isn't surprising at all. The Yield Library was written for GnK. The World Congress alone adds many sources of yields that isn't handled by it.

GrantSP commented 10 years ago

If all the modded files in CEP are the latest, they should take into consideration the WC. The only difference should be the YieldLibrary.

stackpoint commented 10 years ago

That's not how that works.

GrantSP commented 10 years ago

Clearly. Look, As I've said before, I am not 100% aware of the ins and outs of the code. All I know is the user experience now is not up to a standard that deserves to be released. The end user is NOT getting a clear indication of what is happening. Further, those that wish to edit the mod, are not sure the end results of their changes are being displayed correctly.

I'm tired of harping on and on over this issue. I have no idea what to offer as a way to rectify this. I'll will no longer clog this issue with my frustration.

I am confident you have the problem well in hand.

GrantSP commented 10 years ago

Inside the function : City_UpdateModdedYields(city, player) there is a line that queries the Leader.AIBonus.

I see in CEL_End.sql that it is to assist certain leaders that struggle with their AI routines and is also referenced in CEAI_Events.lua with regard to starrting units. I don't think we are using this AIBonus at the moment, does it make a difference whether this stays in or is removed? The log makes a mention of it, but it isn't flagged as an error. It simply says: YieldLibrary.lua:2230: Cannot find key - AIBonus

Perhaps the value should be set to 0 or FALSE at the time of defining it in CAT_AlterTables.sql

e.g. ALTER TABLE Leaders ADD AIBonus boolean default false;