Thalassicus / cep-bnw

Civ V Communitas Expansion Pack
32 stars 22 forks source link

Some text errors #187

Closed Slogun closed 10 years ago

Slogun commented 10 years ago

Only Communitas AI and Tools is loaded.

destroyer

GrantSP commented 10 years ago

Try adding these lines: REPLACE INTO Language_FR_FR (Tag, Text, Gender, Plurality) VALUES ('TXT_KEY_PROMOTION_SEE_INVISIBLE_SUBMARINE', 'Peut Voir sous-marins', '', ''); REPLACE INTO Language_FR_FR (Tag, Text, Gender, Plurality) VALUES ('TXT_KEY_PROMOTION_INTERCEPTION_HELP', '[ICON_AIR] {1_value}% de chances d''intercepter [COLOR_POSITIVE_TEXT] Unités Air [ENDCOLOR]', '', ''); just before PROMOTION_WEAK_RANGED in the _Text_TWPromotionStats.sql.

About the missing majuscules, I'm not sure how we can rectify this easily. The text should have the value, as a %, before the text. e.g. 80% chance to withdraw from melee attacks

I will need to check how the tooltip writer code uses these TXT_KEYs. The code isn't as up-to-date as the rest.

The translations above don't look very good to my untrained eye. You may want to fix that. I also can't guarantee these will actually work, I am not conversant enough with the lua code to verify it. It should work.

Slogun commented 10 years ago

[quote]The translations above don't look very good to my untrained eye.[/quote] I took this screenshot from a full BNW original English game, with CAT only : nothing has been changed.

GrantSP commented 10 years ago

The text is a Google translation of the TXT_KEY from the game files. I am not sure if the fix will actually work. You can try and see if it works. Your French of course will be much better.

stackpoint commented 10 years ago

@GrantSP, the text given in the original post are all in English. @Slogun mentioned that it's a "full BNW original English game"

GrantSP commented 10 years ago

Yeah, a case of partly reading, partly imagining and partly sleeping. So what that still means I think is that those TXT_KEYs are not yet defined properly in _Text_TWPromotionStats.sql. The sql code will still be good. Just change the FR_FR to en_US and replace the French with English.

The SEE_INVISIBLE_SUBMARINE simply says "Can See Submarines." PROMOTION_INTERCEPTION says "chance to Intercept Air Units", which should look okay if it is formatted as [ICON_AIR] chance(%) to intercept [COLOR_POSITIVE_TEXT]Air Units[ENDCOLOR]

I'm redoing all that text anyway, and I'm not really sure which ones display correctly or not.

Like what we are discussing in #114, these new TXT_KEYs as defined in _Text_TWPromotionStats.sql, if they can reuse the original TXT_KEYs as found in the core game I would like to use those instead. Only rewrite them if the variables are missing.

I will have to eventually get down and examine the actual lua to see what is happening, keep putting it off.

stackpoint commented 10 years ago

I don't believe that the original game text for promotions uses use variables at all and it's our custom tooltip writer that does. All the promotions in vanilla BNW is(?) assigned 1 to 1 in terms of text to promotions while our tooltip writer assigns text to promotion effects instead.

GrantSP commented 10 years ago

I was hoping you weren't going to say that. That was what I was thinking, but not sure of. So we need to define every txt that we want to be in the tooltipwriter.

stackpoint commented 10 years ago

There shouldn't be too many promotion effects that need text defined. Around two dozen?

GrantSP commented 10 years ago

And buildings units wonders policies beliefs etc.

stackpoint commented 10 years ago

The tooltip should read the default text for those?

stackpoint commented 10 years ago

Buildings and units should read their stats, policies and beliefs should read their help text since policies and beliefs are fairly unique.

GrantSP commented 10 years ago

Ok. It is sounding better each time.

Slogun commented 10 years ago

Another text problem with Communitas AI and Tools and Communitas Enhanced Gameplay loaded only.

wheat

GrantSP commented 10 years ago

Strange. The tooltip displayed in the 2nd image uses the vanilla TXT_KEY: TXT_KEY_RESOURCE_TOOLTIP_IMPROVED_WORKED When worked by a City:

Both wheat and deer share the same details regarding resources and should therefore share the same tooltip!

InfoTooltipInclude.lua makes use of the strToolTip to call the specified TXT_KEY but I can't see why/where there is a difference made between "deer" & "wheat".

This is going to take me a bit of time. Anyone else with a bit more lua knowledge may find it easier.

Please add anymore anomalies like this to this issue. Thank you @Slogun

I wonder if the same thing regarding the unique units and their lack of tooltips is causing this?

Slogun commented 10 years ago

I wonder if the same thing regarding the unique units and their lack of tooltips is causing this?

I don't think so, as the free units bug seems to be a vanilla French one.

GrantSP commented 10 years ago

As soon as I made the above comment and closed the window I checked again in the database. Wheat is not flagged as giving Food in the Resource_YieldChanges table in CEP.

Somewhere and for some reason wheat is removed as a food yield. This would of course mean it would not show that tooltip.

GrantSP commented 10 years ago

Found it. For some reason, unknown to me at this time, Wheat is deleted from the table as a food! A number of other resources are removed to clear ALL references and then select yields are added back, Wheat is not!

GrantSP commented 10 years ago

Apparently this is one of those changes that you just don't wonder about until it suddenly shows up.

With the initial revamped release of version 3.7 in early November last year this was part of the change as set out in CET_Data.xml line 208. This same file however adds these resources to the Improvement_ResourceType_Yields table. I can only surmise from this that the tooltip shows based on the yields in the Resource_YieldChanges table and not the Improvement_ResourceType_Yields table.

This would mean sheep, deer, banana, fish & whale will display this tooltip but wheat, salt, crab & citrus will not. I will need a bit of outside input into this.

Slogun commented 10 years ago

Salt does display the tooltip. Scouting for more resources...

Slogun commented 10 years ago

Crab & Citrus seem to be ok too. Just had to check Civilopedia : only the wheat won't give any yield.

GrantSP commented 10 years ago

OK. That's it then.

Wheat must just be an omission from the Resource_YieldChanges. Those other resources you have just checked are indeed added back to that table. I guess this means the simple fix is to just re-instate wheat to that table.

GrantSP commented 10 years ago

I guess the additional question here is what do we do when there is a conflict over tooltips?

deer tooltips wheat tooltips

See in the attached images the tooltip that displays when mousing over the resource icon matches the pedia description of the resource but is it necessary to have these when we have the tooltip that shows the yields for the resource on that specific terrain?

Would it not be preferable to not show the basic tooltip? Notice also that in my game there are no worded descriptions of the yield accompanying the deer! I haven't changed my options for some time, is this a difference in detail that the game settings handles?

stackpoint commented 10 years ago

The reason you don't get the total terrain yields when you mouse over the deer icon is because (probably) the only thing being passed to the tooltip reader is that the icon/resource is a deer. And you can't get total terrain yields from just that information.

I don't know why the regular terrain tooltip doesn't have the XXX: prefix in front of it since your screenshots seem to have the fog covering them.

GrantSP commented 10 years ago

The reason for the difference in what each tooltip displays is clear, the reason for keeping both styles is not. If the tooltip that appears over the resource on the terrain gives the info needed, why do we still need the one that displays when mousing over the resource icon? Maybe it is too much trouble to remove, I don't know. All I know is it can cause confusion.

Speaking of, what prefix are you talking of? If it is the Wheat tooltip with no info underneath then that has been fixed by the code I spoke of and is already committed to the source.

stackpoint commented 10 years ago

So you don't want any tooltips when you mouse over the resource icon?

GrantSP commented 10 years ago

If the tooltip for the terrain gives the same detail isn't redundant? Also if you have the options to display the resource icons off isn't it the same, you don't have those tooltips? If so there isn't a lot we will miss.

My preferred option would be to disable the tooltip for the icon and only have the tooltip for the hex tile and have it give details about resource and terrain along with the needed tech to achieve it. As it does now. Perhaps, and this isn't a big issue, change the total to show how much comes from the terrain and how much comes from the resource.

Additionally the only thing the terrain tooltip doesn't show is the happiness from the luxury resources. Not a big deal though, we all should know you get happiness from luxuries.

stackpoint commented 10 years ago

I just looked up the wheat issue and I believe the change was made that a farm would give +2 food as opposed +1 food with a farm and +1 food without it. See http://forums.civfanatics.com/showthread.php?p=11432066&highlight=wheat#post11432066

GrantSP commented 10 years ago

My question would be: if there was a problem with the GetYieldWithBuild function why was it showing correctly for all the other food resources that are given with an improvement? That post is just shy of 2 years ago, the patches in between time could have fixed it if it was so.

At any rate wheat was the only non-showing yield and now it shows. I have a game open and it shows Plains giving 1F & 1P, add the wheat resource unimproved and it is 2F & 1P, improved and it becomes 4F &1P and another 1F if there is fresh water and Civil Service.

croppercapture 2 croppercapture 1

I think, the improved yield should be less food? 3Food?

stackpoint commented 10 years ago

I believe the vanilla functions as wheat having +1 food unimproved and +1 with a farm. Thal modified it so it has +0 food unimproved and +2 with a farm. He probably did this because you don't need a tech to research farming.

So the reason it didn't show a tooltip was because there was no unimproved yield. The resource icon tooltip should probably be modified to state the unimproved yield and the improved yields of the resource.

GrantSP commented 10 years ago

Yes I see that, I guess what I am saying now is the change wasn't a very good one. You can get 1F from plains with no improvements but 2F on grasslands. That change would make it worse to have wheat on plains initially compared to grasslands, which when you look at it are just grasslands with a particular type of grass! Plains with unimproved wheat should be just the same as grasslands. Farming makes that type of grass more productive.

GrantSP commented 10 years ago

Maybe I should move this discussion back to the forum for further debate.

stackpoint commented 10 years ago

Wheat should only spawn on plains and desert tiles. And resources should only have modify positive modifiers on the tile's yields. As in wheat will always have the (+0/+2 food) modifier whether on desert or plains and will only add to the base yield of that tile.

You should probably revert the change unless there's opposition to the +0/+2 modification in the first place.

stackpoint commented 10 years ago

So even if wheat did spawn on grasslands, they would get +4 food with a farm while a plains wheat would get +3 food / +1 hammer and it would be a matter of the base tile instead of the resource itself.

GrantSP commented 10 years ago

It should be viewed in exactly the same manner as fish on a coastal tile. The coast tile gives you 2F, add fish and you have 3F. Now when you send a boat to harvest it you 5F.

If the tile is inside the city's borders it will get a yield even when unimproved.

Wheat on plains is exactly the same.

The only problem I see now is the figures don't look to add up properly. A farm on a wheat/plains tile without fresh water should only give 3F not 4F. Shouldn't it? Unless that was the bug that the change tried to fix. If so the fix is just as bad as the bug. Only for differing reasons.

stackpoint commented 10 years ago

The problem with that comparison is that you have to research several techs and build a workboat to improve the fish tile while all the wheat needs is a worker that can be reused to build several improvements.

The problem with what you did was that you added the +1 food back to wheat but did not reduce the +2 food from wheat farms back to +1 food. There was no technical problem with what @Thalassicus did to the wheat resource. The tooltip problem was that the resource only describes the unimproved yield values and wheat didn't have any. Instead of adding the yield back, we should instead add "+2 food with a farm" into the tooltip.

GrantSP commented 10 years ago

Granted I probably should have removed the extra food from the farm but the initial yield from the tile should not be reduced.

Even without improvements fish on coast give 3F. The difference with them and wheat on plains is not the techs. You still get 3F on coastal fish without having a workboat. You get 5F with boats!

So I do think the unimproved yield value as it was is a mistake. You should get more yield for wheat on plains than what it is/was giving.

The logic of reducing the unimproved yield to below other worse tiles, just so the improved tile yield looks good, is flawed.

Technical problem with the change, no. Logical problem, yes.

So, I will revert the change and wait to see what transpires with the forum discussion.

I suppose we can close this if you have the tooltip code under control?

stackpoint commented 10 years ago

I'm not going to touch any of the tooltip lua code until YieldLibrary is finished.

GrantSP commented 10 years ago

Getting back to the OP. As I work through the text for the Promotions I find the DB actually defines the value for the PROMOTION_INVISIBLE_SUBMARINE as text with the wording shown in the OP. So the TooltipWriter was actually functioning correctly and it pulled the info that was found from the DB. I can add a better description to be shown when this PROMOTION is available.

With help from @Thalassicus, I was able to see more clearly just how powerful this framework of code is. My mistake was seeing the files I was editing as replacements for the entire PROMOTION/BUILDING/UNIT help text. This is wrong! What it actually does is define specific attributes of those things and formulates a tooltip by gathering all the values from the DB. e.g. PROMOTION_BLITZ may only have one feature and it would match the _HELP file description, thus causing me to wonder why the duplication. But! The actual EFFECT of that promotion is what is defined in the new text, not the promotion itself. So, if we gave BLITZ an additional EFFECT of RangeAttackIgnoreLOS, the tooltip would then show this new piece of information also with text like: "Ranged attacks may be performed over obstacles (as long as other friendly units can see the target)". Or similar.

This is tremendously powerful. More so than I even imagined at first. It is however in need of an urgent makeover to care for ALL the areas of the game that display useful information to the player. Units, buildings, promotions, improvements, terrain, trade, combat outcomes, etc. are all areas where the player needs correct information at hand. Whether it is shown via a tooltip, pedia page, popup panel, or something new entirely, the tooltip writer code CAN be plugged into that specific lua and used. The sooner we become familiar with it the better. IMHO. :smile: