Fix in Progress Companion Enemy Enable Fails

[Levis]

The root cause of this bug is that functions don't like being passed attributes as integer arguments. I didn't know that until I tracked down this one, and judging by the lines which were fixed, neither did some other modders. Expect it to happen again when someone else who doesn't know this writes, or copies, a line with a function call which does it.

There are two ways to solve this. One is to check every existing function call and add conversions such as 'sti', then check every new mod and make sure its function calls don't pass attributes without conversions. Another is to make the functions do the conversion themselves. The latter has the advantage of only needing to be done once. It's probably not necessary to check every function; you'll know if the problem is happening because it generates reports in "error.log". I don't recall exactly what it said, something or other about variable type, but that was what led me to the problem and the fix. So, if there's an obscure bug and an error log about variables, that's the time to see about adding something to the function.

having something like this will still screw things up:

string a = 1;
string b = 2;
int sum = getsum(a, b);
logit(a+" + "+a+" = "+sum);

int getsum(int val1, int val2)
{
    int v1 = sti(val1);
    int v2 = sti(val2);
    return v1+v2;
}

You can run it in the console. I think it will still act weird

 

[Grey Roger]

It will probably complain because a1 and a2 have not been defined. But at one point I had this in "nations.c", specifically in the definition of 'HoistFlag':

     if (bPlayerCompanion)
     {
trace("HoistFlag: '" + GetMyShipNameShow(rcharacter) + "' hoists flag " + iNation);
       rCharacter.nation = iNation;
       rCharacter.Flags.DoRefresh = true;
     }
     else
     {
       iNation1 = iNation;
trace("HoistFlag: '" + GetMyShipNameShow(rcharacter) + "' does not change flag");
trace("You are " + iNation1 + ", '" + GetMySimpleName(rCharacter) + "' is nation " + sti(rCharacter.nation));
trace("Relation = '" + GetNationRelation(iNation1, sti(rCharacter.nation)) + "'");
       if(GetNationRelation(iNation1, sti(rCharacter.nation)) == RELATION_ENEMY)
       {
trace("HoistFlag: '" + GetMyShipNameShow(rcharacter) + "' wants to mutiny");
         SetMutineer(rCharacter, true);
         bCompanionMutiny = true;
       }
     }

Before I added everything to do with "iNation1" and both the 'GetNationRelation' lines tried to pass "iNation", the ship wouldn't turn hostile and the trace for "Relation" returned a blank. After I added the "iNation1" bits, it all worked perfectly - the trace for "Relation" showed a valid relation number, the trace about "wants to mutiny" produced a message, and the ship did indeed turn hostile.

 

[Pieter Boelen]

The root cause of this bug is that functions don't like being passed attributes as integer arguments.

That is indeed very true.

For whatever maddened reason, PotC does not store attributes in their original format; it ALWAYS stores attributes as strings.
So if you directly pass an attribute to a function that requires a string as input, that is not a problem.
But if you use any attribute for anything that isn't a string, you always have to convert it back to what it should be.
This is, of course, super-inconvenient, super-annoying and asking for trouble.
Unfortunately it is a stupid effect of how the game engine works that we have to deal with.

There are two ways to solve this. One is to check every existing function call and add conversions such as 'sti', then check every new mod and make sure its function calls don't pass attributes without conversions. Another is to make the functions do the conversion themselves. The latter has the advantage of only needing to be done once. It's probably not necessary to check every function; you'll know if the problem is happening because it generates reports in "error.log". I don't recall exactly what it said, something or other about variable type, but that was what led me to the problem and the fix. So, if there's an obscure bug and an error log about variables, that's the time to see about adding something to the function.

Despite the function taking an int as input variable, indeed there might be no harm in adding a conversion for that variable afterwards anyway, just to make absolutely certain it really IS an integer.

Question with that though: What if you DO correctly supply an integer? Does something like int value = sti(integer); mess up the original integer? How about int value = makeint(integer); ?
Or what if you accidentally supply a 'float' instead of an 'int' or 'string'? Can your suggestion of an extra int value = inpVar; line be trusted to always do the conversion properly regardless of the type of input variable?
There are a lot of different options and I'm not sure which one is the safest of them all....

If somebody knows which one is always OK, then of course we can start doing that.

 

[Levis]

I strongly advice to make sure all function calls are right instead of trying to fix it in the functions.

 

[Grey Roger]

Question with that though: What if you DO correctly supply an integer? Does something like int value = sti(integer); mess up the original integer? How about int value = makeint(integer); ?

Presumably 'sti' means "string to integer". Does it get upset if it is supplied with something other than a string? Does 'makeint' convert strings or floats into integers, and if not both, what does it do if it gets the wrong one?

Or what if you accidentally supply a 'float' instead of an 'int' or 'string'? Can your suggestion of an extra int value = inpVar; line be trusted to always do the conversion properly regardless of the type of input variable?

We already know what happens if you supply a 'float' to a function which is expecting an 'int'. It works. 'SetCurrentTime(int hour, int minutes)' worked perfectly well if you gave it a 'float' for "hour" - in fact, until recently, it only worked if you gave it a 'float' for "hour" because it ignored "minutes" entirely.

We can't make functions idiot-proof because evolution will just produce a superior idiot. But we can cater for the common mistake of supplying a raw attribute to a function which is expecting an integer.

It doesn't always cause trouble anyway. Looking at a few examples, functions which expect an integer, get something else and use it directly seem to be alright. It's when they try to pass it as an integer to another function that the trouble starts. 'HoistFlag' ran into trouble because of calls on 'GetNationRelation(iNation, sti(rCharacter.nation)' when 'iNation' had been supplied as an attribute.

I strongly advice to make sure all function calls are right instead of trying to fix it in the functions.

Good, then presumably you're going to volunteer to check all new code to make sure all their function calls are right.

 

[Pieter Boelen]

Presumably 'sti' means "string to integer". Does it get upset if it is supplied with something other than a string? Does 'makeint' convert strings or floats into integers, and if not both, what does it do if it gets the wrong one?

Those are exactly my own questions. I honestly do not know; somebody would need to try, I suppose.

Good, then presumably you're going to volunteer to check all new code to make sure all their function calls are right.

If any mismatches are found, it is not all that difficult to double-check all function calls and correct them if necessary.
Checking all new code in advance is a different story though. I do that to some extent because I always WinMerge every code from anyone else that goes into my game.
But while that does provide some measure of quality control, it is still extremely easy to miss mistakes with the code anyway.

 

[Levis]

Good, then presumably you're going to volunteer to check all new code to make sure all their function calls are right.

working on it

 

[Pieter Boelen]

I've now added 'sti' to all HoistFlag function calls that were missing them.
That should also fix the bug correctly and therefore @Grey Roger's "failsafe" code should no longer be necessary.
Therefore I am excluding that from my game, though it also couldn't harm being included.