Learning by doing can clear experience

[Nightinggale]

There is a feature called "Learning by doing" where units remembers how many turns it has worked in a certain profession. If it has worked enough turns it gets a chance to become professional.

There are a few problems though.

1. it's invisible in the GUI
2. it's an int and it's reset if the profession changes
3. the AI is moving units around in colonies way too often to make use of this

This ticket is about 2. Imagine you have a free colonist set as a fisherman and you want him to become an expert fisherman. After having fished the same plot for 20 turns, a pirate move past, forces the colonist to a new job and then leaves. You can then place the colonist on the water again, but he just lost 20 turns of experience by being moved.

We should also consider what we want with this feature. Should we use the journeyman modcomp? It's basically the same idea, but it creates a trained person, who has +1 production instead of going directly from free colonist to expert.

[abmpicoli]

I could swear that I had made an issue "idea" to change the learn by doing code... But can't find it... Probably I've forgot to "save" it... :)

Option 1: My idea for thatis that units that can "learn by doing" should have a "cache" of professions they have worked previously, and for how many turns... I think a cache of 6 professions is a good balance between performance and functionality. When the unit change to a profession that doesn't exist the cache, the cache entry with the smaller experience goes out.

Option 2: add into the interface a way for the player/AI to lock a profession for "learning by doing". Say , we can lock the colonist to only accept "learn by doing" for the fisherman profession.

Both options 1 and 2 demand changes in the save structure (to save the "lock" or "caches" ).

Option 3: add into the interface/AI a way for specifying to which profession the free colonist wants to become expert. If they change professions, those turns simply wouldn't count for it's expertise... This is save compatible: we simply change how the profession he is learing is filled. If the player/AI doesn't define the profession, then the profession will simply be the first one the colonist is assigned to. We could make the "clear specialty" button to reset the learn by doing training as well..

Option 4: add a "decay" concept. Whenever the player switch professions, instead of reset the old profession, it makes the progress towards expertise be decreased by a specified amount. When the progress reaches 0, the profession to which it is becoming expert changes. This is save compatible, so it is a good option as well. However, it have the downside that training will take much longer...

[abmpicoli]

For "Option 1" I've also thought that we could make the rule to be (to have, say, 20 turns on the specific profession or 10 turns at the specific profession and 15 at other professions...)

[Nightinggale]

For option 1, we could also add a ProfessionArray. It will automatically grant an array of length num professions. In doTurn, call the array with add(1, eCurrentProfession) as long as eCurrentProfession != NO_PROFESSION. It's not the most efficient usage of memory, but it minimize CPU cycles.

Don't worry too much about the savegame. I know a lot of tricks to preserve them if needed.

[devolution79]

I need to create a ticket for this, but I actually overhauled the UI a bit so that it can now show the odds of becoming an expert along with some other invisible/hidden unit stats. It's sitting in yet another branch...

[abmpicoli]

I'm implementing a few lines change in the code that I think will make leard by doing more effective. Basically, learn by doing is never reset, either for humans or AI. If professions change, only at that turn , there is no progression , and the chance of becoming expert is zero.

Also, the chance of becoming an expert by hard work is never over 30% every turn (making that when we place that statesman as a fisherman there is a chance that he doesn't become a fisherman in the next turn).

I will let you know what happened...

[abmpicoli]

Hi, everyone... Would someone confirm with me something? I've just discovered that the formula for Learn By Doing have an error, that I think is related to the loading of the GlobalDefinesAlt.xml ... At the xml it is clear that this parameter

        <DefineName>LBD_CHANCE_INCREASE_EXPERT</DefineName>
        <iDefineIntVal>1</iDefineIntVal>
    </Define>

should be one. However what is feed to the LbD routine is zero. Because of that, the chance of someone become an expert is really slim : after 30 turns at normal speed, the chance of becoming an expert every turn is 3,6 to 4,5 % every turn after 30 turns... because the "increase" in the formula is zero. https://github.com/We-the-People-civ4col-mod/Mod/blob/a430fb062ad2539c42142f1c563f9dc9c866d0ea/Project%20Files/DLLSources/CvCity.cpp#L10847

I tried to check the CvGlobals.cpp / CvGlobals.h but I can't find anything wrong with the code... The same code that specifies that base chance is 36/45 is used to get the chance Increase...

https://github.com/We-the-People-civ4col-mod/Mod/blob/a430fb062ad2539c42142f1c563f9dc9c866d0ea/Project%20Files/DLLSources/CvPlayer.cpp#L20032

[Nightinggale]

https://github.com/We-the-People-civ4col-mod/Mod/blob/a430fb062ad2539c42142f1c563f9dc9c866d0ea/Project%20Files/DLLSources/CvCity.cpp#L11041

This line looks wrong. The comments says it's supposed to scale to game speed, but it becomes 1 / 100 / 100. chance_increase_expert is indeed 1 when reaching this line and it's 0 afterwards. It is then used as argument to become the increase variable.

base + (workedRounds - pre_rounds) * increase / l_level <-- none of those variables use the i prefix, hence aren't complying to the syntax standard. That's not a bug, just sloppy coding.

[abmpicoli]

Mod/Project Files/DLLSources/CvCity.cpp

Line 11041 in a430fb0 chance_increase_expert = chance_increase_expert / train_percent / 100;

This line looks wrong. The comments says it's supposed to scale to game speed, but it becomes 1 / 100 / 100. chance_increase_expert is indeed 1 when reaching this line and it's 0 afterwards. It is then used as argument to become the increase variable.

base + (workedRounds - pre_rounds) * increase / l_level <-- none of those variables use the i prefix, hence aren't complying to the syntax standard. That's not a bug, just sloppy coding.

Yep, you have just found the issue... it should be chance_increase_expert * train_percent / 100 ... and it must be at least one.

[abmpicoli]

Edit: the TRAIN_PERCENT gets bigger at slower speeds. So it must be a multiplier for the "pre-rounds" thing, and a divisor for the chance_increase.

[abmpicoli]

I'm also making another change : it doesn't make sense that civilizations with traits that increase the likelyhood of learn by doing doesn't get a lower pre_rounds_expert... I'm also making the LBD_CHANCE_INCREASE_EXPERT=5 . it will make the value to be different with each game speed.

[devolution79]

Sounds like a sensible change. Btw, while you're at it, maybe we should refactor the function so that we separate the lbd odds computation from the execution\upgrade. Not only will it make the code cleaner but it is needed for my change to the gametext that "unhides" the hidden learning parameters. I'll make a ticket for it so that it's easier to understand what I'm proposing.

[abmpicoli]

Oh, got it. expose a function like "LbD_calculateUpgradeOdds()"? Possibly tied to the unit... And perhaps do a LbD_calculateEscapeOdds() ?

[devolution79]

See #233

[abmpicoli]

See #233

yep

[abmpicoli]

One thing I have been wondering about with issues like this is to merge display code and stats calculation like adding a CvString pointer to the arguments. If the string pointer isn't NULL, it will fill in each parameter to the string as they are calculated. This will ensure a fast way to get the final modifier (set string to NULL) and at the same time it prevents the display code from going out of sync with the calculations.

Maybe it's not a huge issue here, but it is in some locations and we might as well figure out a standard for doing this everywhere. If we always do it like this, we get fairly clean code and avoid display bugs in the future.

I believe this "calculateOdds" thing should be an int, and display should be adjusted by python code... This way we clearly separate logic from front-end formatting...

[Nightinggale]

I believe this "calculateOdds" thing should be an int, and display should be adjusted by python code... This way we clearly separate logic from front-end formatting...

I moved the post itself to #233. Besides the current system is actually to get the DLL to generate a string. Currently the DLL has one function to generate the string and one to calculate modifiers. Say for instance the help for food production for a colonist working as a farmer. The displayed production and modifiers is one function and the production during doTurn() is another function and you just pray those two will not go out of sync. M:C has had a bunch of asserts due to this problem and once it starts asserting, it's far from easy to fix.

[raystuttgart]

Is there still anything wrong with Learning by Doing? I could swear it was working nicely in RaR. (I had Units become Experts several times.)

If there still is a problem, please let me know what it is exactly. I would then also fix it of course. If there are no more bugs / problems, we could also close this.

[Nightinggale]

The issue is that if you have say a free colonist working as a fisherman and a pirate shows up, then the colonist might work as a stateman as the plot becomes unavailable and you move the colonist back when the pirate is gone.

While this is perfectly fine (though a little annoying), changing profession will reset the turn counter for LbD. Ideally the counter would be in an array meaning doing a single turn of something else wouldn't completely clear the original counter.

Also if you have two free colonists working on a building, you can't tell which one was just added and which one has been working for 20 turns.

Other than that it seems to be working fine. If you do not interrupt the learning process by moving the colonist, then it will eventually become an expert.

[raystuttgart]

Ah ok, then there is no big issue with feature itself clearing "LbD"-experience. It is just caused by Wild Animals and Pirates forcing the Unit to changes its Profession.

By the way: A) The Visualizsation of turns the worker has worked in his job is easy to do in Mouse Over. B) The "restet when worker changes job" was done purposely by me because I wanted to have it "use this worker permanently in his job if you want him to become an Expert." (It is at least a tiny bit challenging that way - and no simply freebee)

Thus: A) I will just implement it if wanted. (Should not be difficult.) B) I am not even sure if I want it. (Because the feature would become too easy.)

[Nightinggale]

B is needed because of the animal/pirate issue. However we could add a counter to make the unit forget the experience after say 3 turns and when finding a profession, the unit will prefer the profession the unit has experience with. That way you don't get experience from moving a unit around all the time, but at the same time a unit will not have to start over just because an animal passed through for a single turn.

[raystuttgart]

B is not an "issue" for me. Yes, they will mess up your LbD experience. But that just results in Animals / Pirates being a little bit more punishing.

Everything that adds a little bit of negative consequence does not always need to be called an issue. Games do not always need to be sunshine and happiness, because that would get boring.

[devolution79]

If anybody goes ahead and implements the gametext change, then please keep this in mind: https://github.com/We-the-People-civ4col-mod/Mod/issues/233 (also check out the code in the branch) We may want to overhaul all of the text since we're hiding a bunch of useful info

[raystuttgart]

Great, I just implemented point A. :( I can throw my change away then.

Because it basically does nothing else than #233 (The number of turns having worked in Profession is always displayed though - because that should be no "secret".)

@ErikSandbraaten However it seems as if I implemented my change at a different position though. Because I did not see your code (or at least no comments for it). Is your change pushed to "fixes"?

I implemented here in this function: void CvGameTextMgr::setCitizenHelp

Just for comparison this is my code: (It is quite simple.)

// WTP, ray, showing LbD turns for Profession in Citizen Help - issue #77 - Start
int iTurnsWorkedInLbDProfession = kUnit.getLbDrounds();
if (iTurnsWorkedInLbDProfession > 0)
{
    szString.append(NEWLINE);
    szString.append(gDLL->getText("TXT_KEY_MISC_HELP_LBD_TURNS", iTurnsWorkedInLbDProfession));
    szString.append(SEPARATOR);
}
// WTP, ray, showing LbD turns for Profession in Citizen Help - issue #77 - END

[devolution79]

The code is in the display_hidden_parameters branch

[devolution79]

This is our chance to merge all this stuff together. We should also refactor the LBD stuff while we're at it. My code also calculates the odds which I think is handy as well.

[devolution79]

Just a quick note @raystuttgart , you should always check a bit for prior art since there is quite a bit of stuff lingering around and we don't want to implement things twice or even thrice!

[raystuttgart]

Ah ok, then it is obvious why I did not see it.

Small improvement suggestion to your code: (which is probably better than my simple solution)

The number of turns the worker has worked in current Profession can always be displayed. (There is no need to keep this secret it will always help the Player.)

I will throw my change away.

And you are right, I should have checked better for other stuff. It was my fault.

I am currently just a bit confused because we commit to so many different branches.

[raystuttgart]

Just a few Explanations:

• For AI Players the "LbD experience" is not reset even when it changes to another Professions. Thus AI does not have a problem with "resetting LbD experience".
• The time for becoming an Expert depends on the Profession (higher level Professions take longer).

[Drunk-Monk]

B is not an "issue" for me. Yes, they will mess up your LbD experience. But that just results in Animals / Pirates being a little bit more punishing.

Everything that adds a little bit of negative consequence does not always need to be called an issue. Games do not always need to be sunshine and happiness, because that would get boring.

However this also occurs if the AI decides to move a unit inside your settlement, or you move a unit by accident. The player should not be punished for that. We don't want a system, where a unit can become a 'master of all trades', but we should have the experience decline, or have a grace period rather than a sudden fall.

[raystuttgart]

Well an "experience decline" would be doable as well. But that would mean some effort for reprogramming.

For me this is currently not worth the effort - it is simply too small of an issue for me. But I don't mind if anybody else wants to further improve LbD of course.

[Nightinggale]

Implemented