Fixed Skills: System Rewrite for Performance

[Pieter Boelen]

Oh, and @Levis: You can skip doing a lot of work on the skill calling performance.
After thinking about it some more, I think I should be able to do further work on this later today.
But please do look at my rewritten code and let me know if you think anything doesn't look right.

Also, I could use some pointers on the correct spots to add in automatic recalculate points.
Are there any specific places where the following are handled?
- Game automatically adds a new skill point of a certain type
- A character's officer type is changed (probably through the Passengers instead or perhaps dialog)

I'd like to limit the recalculation to only those skills that actually changed wherever possible.
For example, when an officer gains Sailing, reset and recalculate only that skill for that officer's Captain.
In other words: I want to reduce this down to specifically only that what needs updating.

Of course changing officer type does need to go through all skills, but so be it.

 

[Levis]

The changing officertype will be hard to pick up because it's been done manually a lot instead of using a function. I do have an function in place which picks up if a officertype is changed on load.
A place where you should add a recalc at least is in the while Loop for the addskillxp (or something like that) in leveling. It Triggers when a skill Levels up.

I will look at the code later today. But I will try to decrease the load times instead then.

 

[Pieter Boelen]

The changing officertype will be hard to pick up because it's been done manually a lot instead of using a function. I do have an function in place which picks up if a officertype is changed on load.

Really? Shouldn't we then make some sort of ChangeOfficerRole(ref character, int OfficerType) function that we can call whenever we actually do that?

I will look at the code later today. But I will try to decrease the load times instead then.

 

[Levis]

Really? Shouldn't we then make some sort of ChangeOfficerRole(ref character, int OfficerType) function that we can call whenever we actually do that?

Would be nice if we had that function. but you Need to Change it at a lot of places then.
OfficerType is a string btw .

 

[Pieter Boelen]

Would be nice if we had that function. but you Need to Change it at a lot of places then.

Lot of places? Why?
I can think of only three:
1. Passengers interface as called by the "Change Officer Role" button
2. Enc_officer_dialog.c as you can change officer role there too
3. Wherever characters are first initialized as I understand now ALL characters get an "officer type" of some kind assigned

Where else would officer types for the same character be changed into another one?

OfficerType is a string btw .

Ah, of course it is. Was writing from memory.

 

[Levis]

Lot of places? Why?
I can think of only three:
1. Passengers interface as called by the "Change Officer Role" button
2. Enc_officer_dialog.c as you can change officer role there too
3. Wherever characters are first initialized as I understand now ALL characters get an "officer type" of some kind assigned

Where else would officer types for the same character be changed into another one?


Ah, of course it is. Was writing from memory.

hmmm right maybe ist not that much BUT
A lot of characters aren't define with theire officertype and they are only set later in the questcode. so you'd have to take those cases too.

 

[Pieter Boelen]

A lot of characters aren't define with theire officertype and they are only set later in the questcode. so you'd have to take those cases too.

I suppose I can do a search for those. We'll see....
In any case, if you say there isn't such a function, we really should make one.
Where might I find the code that should be executed when a character's officer type is modified?

 

[Levis]

I suppose I can do a search for those. We'll see....
In any case, if you say there isn't such a function, we really should make one.
Where might I find the code that should be executed when a character's officer type is modified?

It's in leveling.c
search for officertyep and/or Change and I think you'll find it. It's somewhere in an other function. I believe it calls the initcharacterskills function when a offictype is changed.

 

[Pieter Boelen]

@Levis: Had a long day today, so I won't jump into the deep again on this one just yet.
It would probably be wise to allow you a chance to look through the code changes I made so far as well.
Maybe you can think of a reason why this concept isn't actually a good idea, in which case I shouldn't spend a lot of time and effort at it.
So please have a look at the code itself and let me know what you think!

 

[Levis]

Lol it seems in the past this system was already in place

        if(CheckAttribute(arItm, "skill") && !CheckAttribute(_refCharacter, "skiprecalcskill")) // NK 05-06-27 for auto change of 
        {
            for(int sn = 0; sn < 10; sn++)
            {
                DeleteAttribute(&_refCharacter, "skill." + GetSkillName(sn) + ".mod");
                // Aconcagua: what was this called for???
                //GetPartySkill(&_refCharacter, GetSkillName(sn));
            }
        }

But someone removed it .
This is from TakeNItems

 

[Pieter Boelen]

Indeed! Looks like that removes the attributes to be calculated again later.
Of course that would only work if they ARE being recalculated before they're needed the next time.
Not sure if that was ever done, but it could be added. I'll try for sure.
But only for those skills that are actually being affected.

 

[Levis]

@Pieter Boelen if you want I can make the script to recalc them etc tonigh so we got that functionality working agian also.
Tomorrow I have a bit more time to work on it and after that we could post an update again. It should slowly become a very stable release which is well playable also.

 

[Pieter Boelen]

I am actually looking into this right now. Hopefully I can get at least some basis functionality restored.

 

[Pieter Boelen]

HA! It totally works through TakeNItems!
As soon as you receive the item, it removes the "modifiers" for ONLY the skills affected.
When the game next needs those modifiers again, ONLY those will be recalculated.
Quick test with one of the No Save monks (thanks again @pedrwyth! ) indicates that it works without calling UpdateSkillModifiers .
So far I didn't notice any performance drop because of it either, but of course it'll be interesting to see what happens later in the game.

 

[Levis]

Ok if you fix this then I'm going to look a bit more into load times and start on the merchants soon.

 

[Pieter Boelen]

Ok if you fix this then I'm going to look a bit more into load times and start on the merchants soon.

I'm going to ask you to figure out the ChangeOfficerRole function call as I dare say you're more familiar with where that is done than I am.
Had a quick look, but it would probably take me a lot more time to figure out how and do it than for you.

In the meantime, I got all the files I changed together in the attached ZIP. Be sure to WinMerge these with your game!
I added several comments to the new added functions AND in the spots where I think a function call needs to be added by you.

Apart from those spots, everything seems to be operational as far as I could tell from my (VERY!) quick test.
I am hoping to make another EXE release on Thursday. If we both did our jobs well, that should become the most playable version in a LONG time!

 

[Levis]

So @Pieter I think I've found a very elegant way of fixing this:

int CalcSkillModifier(ref character, string skillName)
{
    int j, qty;
    int mod = 0;
    ref itm;

    int iChrCabin = FindCharacterShipCabin(character); // KK
    for(j = 0; j < SKILLITEMS_QUANTITY; j++)
    {
        itm = &Items[NativeFindCharacter(&Items, SkillItems[j].id)];
        if(!CheckAttribute(itm, "skill." + skillName))
            continue;
// KK -->

        string itmid = itm.id;
        qty = GetCharacterItem(character, itmid);

        if (iChrCabin >= 0 && CheckAttribute(&Locations[iChrCabin], "box1.items." + itmid) == true)
            qty += sti(Locations[iChrCabin].box1.items.(itmid));
// <-- KK
        if( qty >= sti(itm.skill.num)) // *.skill.num = num required for effect. Default = 1
        {
            if(CheckAttribute(itm, "groupID") && IsCanEquiping(character, itm.groupID) && !IsEquipCharacterByItem(character, itmid))
                continue; // if equipable and not equipped, skip // KK

            // *.stack = num of item that can be stacked together. Default = 1
            if(sti(itm.skill.stack) >= qty/sti(itm.skill.num))
            {
                mod += sti(itm.skill.(skillName)) * qty/sti(itm.skill.num);
            }
            else
            {
                mod += sti(itm.skill.(skillName)) * sti(itm.skill.stack) / sti(itm.skill.num);
            }
        }
    }

    mod = iclamp(-MAX_SKILL_INCREASE, MAX_SKILL_INCREASE, mod); // PB: Single line
    character.skill.(skillName).modifier = mod;
    character.skill.(skillName).officertype = GetAttribute(character,"quest.officerType"); //Levis: we also store for which officertype this is set, so when it's changed we know to change the mod too <------------------------ THIS ONE
    return mod;
}


// Effective personal skill, based on officer type
int GetEffectiveSkill(ref character, string skillName)
{
    int skillPoints = 0;
   if(GetAttribute(character,"skill."+skillName+".officertype") != GetAttribute(character,"quest.officerType")) ResetEffectiveSkill(character, skillName); //This detects if the characters officertype has changed<------------THIS ONE
    if (CheckAttribute(character, "Skill." + skillName + ".effective"))
    {
        skillPoints = sti(character.Skill.(skillName).effective);
    }
    else
    {
        skillPoints = CalcEffectiveSkill(character, skillName);
    }
    return skillPoints;
}

I marked the lines which are added

 

[Levis]

This version of the zip should have the skill system fully operational again. Also trying to improve loadtimes a bit more.
When you use this version you will hopefully notice the loadingtime to the main menu is shorter. Also working on decreasing the loading time for a new game and location loads.
Most notably I've added a timer which can be used to check performance of different functions. At the moment it's set up for the game load. Please keep this for now and please post some of your compile logs after an new game is started so I can compare them with mine and see if some functions might be more taxing for others (worst computer I have here has 8 cores so might be a bit biased ). Note also that the times in the compile log aren't the real times. They are normalized cpu cycles. If your only using one cpu which is fixed at a certain speed they will be real times. I think you can multiply the number for total runtime times the amount of cores you have to get an estimation but I'm not sure.

 

[Pieter Boelen]

So @Pieter I think I've found a very elegant way of fixing this:
[...]
I marked the lines which are added

Clever! That seems to work in most cases and probably without added adverse impact on game performance.

Though it seems I also already managed to break it with the attached savegame (Standard Storyline) that I was testing with before.
1. Hire the guy in front of you (First Mate): All skill values seem OK after hiring.
2. Set him to Navigator: His skill contributions correctly update
3. Change him back to First Mate: Contributions do NOT correctly go back!

Some other things occurred to me that we still need to address:
- "Fleet Mali" are applied only to the PARTY, so you could see it in the Passengers interface but NOT in the Character interface; that might make it not so obvious to notice
- "Fleet Mali" do not appear to work (got my Leadership 2 character a Tier 3 ship, but the Party Skill value in the Passengers interface still indicated 2 and isn't reduced)
- Party Skill has to be reset when you get a new ship in any way, because this may affect the "Fleet Mali"
- Does "Remove from Post" work? That one DELETES the "officertype" attribute!
- Does Auto Skill System OFF adding skill points also update everything correctly?
- ResetPartySkill uses FindCaptainIndex; does that work properly for NPCs or will that do weird things?
- We have to check all calls to GetPartySkill to check if those should not be replaced with GetEffectiveSkill or CalcCharacterSkill (depends on whether it is personal or not)

Those are the main things that spring to mind right now. There may be more....

 

[Pieter Boelen]

Also:

- Use officer-type effective skill for NPC captains or have them equal to the player and contributing ALL skills?
Seems fairer to have them contribute everything and have their officer type used only to determine what skills to prioritize on level-up.
Alternatively, perhaps apply the captain-type dependent contributions to the PLAYER as well?
That way even a maxed out player would STILL require officers to assist. Probably the most realistic, but may be controversial....