Fix in Progress Companion Enemy Enable Fails

[Grey Roger]

Good news: I've nailed a bug. If the player is going to raise a hostile flag and attack the French ships, he's probably not going to wait - he's going to do it right after he sets sail from Isla Mona, while they're still attached. That doesn't work, and my guess is that the mod to allow you to double-cross a ship you're escorting by raising a hostile flag without pre-warning the captain won't work either. This line, part of HoistFlag(int iNation), is the culprit:

if(GetNationRelation(iNation, sti(rCharacter.nation)) == RELATION_ENEMY)

I'd put a trace there to display the values for 'iNation', 'sti(rCharacter.nation)' and 'GetNationRelation(iNation, sti(rCharacter.nation))', and what it displayed for 'GetNationRelation(iNation, sti(rCharacter.nation))' was nothing. Not 0, nothing - blank. The reason turned out to be that the system doesn't like argument 'iNation' being passed directly to another function. I defined a new integer variable 'iNation1', set it equal to 'iNation', then passed that to 'GetNationRelation(iNation1, sti(rCharacter.nation))', and now if I raise a hostile flag while the French ships are companions, they turn hostile - and an escorted merchant will now probably do likewise if you raise a hostile flag, though I've yet to check that. (And also check that it does not turn hostile if you raise a hostile flag after pre-warning the captain.) The modified version of "nations.c" is also attached.

 

[Pieter Boelen]

Good news: I've nailed a bug. If the player is going to raise a hostile flag and attack the French ships, he's probably not going to wait - he's going to do it right after he sets sail from Isla Mona, while they're still attached. That doesn't work, and my guess is that the mod to allow you to double-cross a ship you're escorting by raising a hostile flag without pre-warning the captain won't work either. This line, part of HoistFlag(int iNation), is the culprit:

Yikes, NOT good news! I know for 99% certain that feature used to work correctly, because I had to add extra code in place to prevent it (the telling Escort ships that you'll be using a false flag bit).
Now I wonder how long ago that broke; can't have been much more than 1.5 years ago, I reckon.

I'd put a trace there to display the values for 'iNation', 'sti(rCharacter.nation)' and 'GetNationRelation(iNation, sti(rCharacter.nation))', and what it displayed for 'GetNationRelation(iNation, sti(rCharacter.nation))' was nothing. Not 0, nothing - blank. The reason turned out to be that the system doesn't like argument 'iNation' being passed directly to another function. I defined a new integer variable 'iNation1', set it equal to 'iNation', then passed that to 'GetNationRelation(iNation1, sti(rCharacter.nation))', and now if I raise a hostile flag while the French ships are companions, they turn hostile - and an escorted merchant will now probably do likewise if you raise a hostile flag, though I've yet to check that. (And also check that it does not turn hostile if you raise a hostile flag after pre-warning the captain.) The modified version of "nations.c" is also attached.

Yikes AGAIN! Of course that change should make absolutely zero difference because it is perfectly valid syntax to directly use the 'iNation' variable.
"Should" being the defining word here, because I've certainly tracked down bugs before that had completely ridiculous root causes.
I remember one time that a valid line of code served as a "garbage collector" of sorts and just complete nonsense ended up happening there.
So to "fix" it, I put a dummy line of code above it so the "garbage" got collected there and the subsequent line would actually behave itself properly.

Scary stuff, because this means that even 100% valid code that looks completely correct can STILL mess up in absolutely inexplicable ways.
And the only way of finding out is to first notice something is wrong and then do some seriously in-depth debugging like what you did there as well.

In any case, good catch and thanks for finding a solution!

 

[Levis]

I haven't been able to keep up with this discussion. @Pieter Boelen if you need me to look over something I assume you tag me. Else I hope you've got this one as you know this part way better then I do.

 

[Pieter Boelen]

I haven't been able to keep up with this discussion. @Pieter Boelen if you need me to look over something I assume you tag me. Else I hope you've got this one as you know this part way better then I do.

@Grey Roger found another error along with a proposed fix. However, his solution should in theory make absolutely no difference, but fixes it anyway.
I've had something very similar before and it's quite scary because it means we cannot trust the game code to always follow regular programming logic.

 

[Levis]

might be some bracets are places wrong or there is some weird stuff going on with variables being define wrong?

 

[Pieter Boelen]

might be some bracets are places wrong or there is some weird stuff going on with variables being define wrong?

If only it were so simple, but this one is REALLY crazy. And it isn't the first time we've had insanity like this.

Basically, this doesn't work right (below code vastly simplified from its actual version):

void HoistFlag(int iNation)
{
if(GetNationRelation(iNation, sti(rCharacter.nation)) == RELATION_ENEMY)

But this does:

void HoistFlag(int iNation)
{
int iNation1;
iNation1 = iNation;
if(GetNationRelation(iNation1, sti(rCharacter.nation)) == RELATION_ENEMY)

Only difference is the intermediate 'iNation1' variable.

 

[Grey Roger]

Scary stuff, because this means that even 100% valid code that looks completely correct can STILL mess up in absolutely inexplicable ways.
And the only way of finding out is to first notice something is wrong and then do some seriously in-depth debugging like what you did there as well.

I can't remember specific examples but I've seen weirdness of this sort when writing the storyline. It's the first time I've seen it in general game code, though.

It's also possible there's something weird in my install, so try this with the original version of "nation.c", not my fixed version. Start a free-play game, save game outside the tavern, ask about a job in the tavern, and if the tavernkeeper says there isn't one then reload the savegame and try again until you get an escort quest. Don't tell the captain anything about false flags, go to sea, raise an enemy flag and see if he does in fact turn hostile. If not, look in "error.log" for anything relating to "nation.c".

 

[Levis]

Could it be the call is done by HoistFlag(&Nation)?
In that case a pointer is send instead of a value which might cause the problem when transfering it.
Else look what value is presented in GetNationRelation in both cases you can find it in nations.c

It could also be because the nation which is send is a define which doesn't have a variable typ specified perse. So using makeint(iNation) would be safer and should work. I can see why the game might do weird things with this.

 

[Levis]

I also see calls like this

HoistFlag(PChar.nation);

So here a string is send to HoistFlag instead of a int. So that could verywell explain why it's going wrong ...

 

[Pieter Boelen]

I also see calls like this

HoistFlag(PChar.nation);

So here a string is send to HoistFlag instead of a int. So that could verywell explain why it's going wrong ...

Indeed that probably doesn't help; would be better for a 'sti' to be put arround there.

 

[Levis]

Here is a overview of wrong hoisflag calls:

HoistFlag(PChar.nation);

PROGRAM\interface\NationRelation.c (line 480)

HoistFlag(PChar.orgnation);

PROGRAM\SEA_AI\sea.c (line 388)

HoistFlag(PChar.previous_flag);

PROGRAM\Storyline\standard\quests\both_reaction.c (line 164)

 

[Pieter Boelen]

@Grey Roger: Could you replace those three function calls by adding sti() around them like this?

HoistFlag(sti(PChar.nation));

HoistFlag(sti(PChar.orgnation));

HoistFlag(sti(PChar.previous_flag));

Then revert your change in nations.c and see if this also serves to fix it without adding the extra line.

 

[Pieter Boelen]

Start a free-play game, save game outside the tavern, ask about a job in the tavern, and if the tavernkeeper says there isn't one then reload the savegame and try again until you get an escort quest. Don't tell the captain anything about false flags, go to sea, raise an enemy flag and see if he does in fact turn hostile. If not, look in "error.log" for anything relating to "nation.c".

Could somebody test the above in their own game version without first making any changes?
I'd be most interested in a test on the 28 July 2016 version, but one with the latest ZIP would also be good.

If indeed you can replicate the bug, make the changes suggested in posts #11 and #12 above, then try again.
If @Levis' suspicion is correct, that should fix it.

 

[Grey Roger]

@Grey Roger: Could you replace those three function calls by adding sti() around them like this?

HoistFlag(sti(PChar.nation));

HoistFlag(sti(PChar.orgnation));

HoistFlag(sti(PChar.previous_flag));

Then revert your change in nations.c and see if this also serves to fix it without adding the extra line.

Isn't it just easier to leave the change in "nations.c" so that it works? Or, if I am to amend the various files which contain those lines, should I do it to the 28th July versions, the 7th October versions (not installed on my game PC) or the 2nd October versions (probably not installed on anyone else's PC)?

 

[Pieter Boelen]

Isn't it just easier to leave the change in "nations.c" so that it works?

At the very least, I'd quite want to know if these three changes do indeed work to solve the problem as well.
If so, then that explains why it wasn't working in the first place. But if not, then the mystery still remains.

If possible, I do prefer having the function calls be correct instead of correcting the variables after the fact.

 

[pedrwyth]

If no-one is in a rush to consolidate this (given @Grey Roger has a working "solution/workround" to allow progression of his storyline) then I'll take a look at it, partly because I have a working july version to use and partly because reading this led me to realise why I had to do this in transfer_cannons.c

int pwtemp
....
pwtemp = GetLocalShipAttrib(enship, &EnshipRef, "MaxCaliber");
               
            SetCharacterCannonType(xi_refCharacter,GetCannonByTypeAndCaliber(sCannonType, pwtemp));

because GetLocalShipAttrib returns a string but the second input term for GetCannonByTypeAndCaliber is an integer. I guess I was lucky it worked at all maybe just that the string has a #define value which is an integer saved absolute chaos (perhaps) - actually that was part of the "overly complex" coding structure I referred to when posting the file. I will now go away and try an sti() based version and if effective replace the previous one. After that I will take a look at the HoistFlag one - unless someone else gets there first.

 

[Pieter Boelen]

Brilliant, thanks @pedrwyth!

I was going to suggest using this, but reading the second half of your post, it sounds like you already intend to try just that.

SetCharacterCannonType(xi_refCharacter,GetCannonByTypeAndCaliber(sCannonType, sti(GetLocalShipAttrib(enship, &EnshipRef, "MaxCaliber")) ));

 

[Grey Roger]

At the very least, I'd quite want to know if these three changes do indeed work to solve the problem as well.
If so, then that explains why it wasn't working in the first place. But if not, then the mystery still remains.

They do.

If possible, I do prefer having the function calls be correct instead of correcting the variables after the fact.

True, and changing those particular calls makes it work for now. The next modder who feeds an attribute directly to "HoistFlag" is going to trigger the same problem again. Alternatively, just leave "nations.c" with the alteration I made, and for the sake of a couple of lines of code and one variable, it's robust against a similar mistake ever being made again. It wouldn't be the only function which takes precautions against that.

It's also a bit disheartening to fix a problem like this and then have people decide to rewrite it, rather than move onto the next problem. Let's pretend I've written a fix for the problem whereby a flagship which surrenders causes the whole fleet to turn neutral. Now someone else can write the fix the way they'd like to see it and I don't need to bother.

 

[Pieter Boelen]

They do.

Thanks for testing! Then at least we know for sure now why it went wrong and that's definitely a very good thing to know.

True, and changing those particular calls makes it work for now. The next modder who feeds an attribute directly to "HoistFlag" is going to trigger the same problem again.

Indeed. And that is why I'm actually quite torn on what would be the best thing to do.
It is either a robust function with additional failsafes (using additional code that in theory should not be necessary)
OR it is a clean function, but whoever makes use of that function is required to get the syntax right for calling it.

Since virtually all functions currently in the game don't have any of those failsafes, should they be added to all functions?
At the moment I honestly do not know.

It's also a bit disheartening to fix a problem like this and then have people decide to rewrite it, rather than move onto the next problem. Let's pretend I've written a fix for the problem whereby a flagship which surrenders causes the whole fleet to turn neutral. Now someone else can write the fix the way they'd like to see it and I don't need to bother.

Since you are referring to that specific example there, I can honestly say that suggestion basically meant
to deliberately break correct code because that working code triggers a bug elsewhere.

Personally, at the very least, I want to know exactly WHY a bug occurs before making a final fix.
Without knowing that, there is no way to know for sure it won't cause weirdness elsewhere or at a later date.

In the end though, if you guys are comfortable using bandaid fixes and hiding bugs instead of finding their root cause, be my guest.
It is certainly easier and quicker to do it like that. And I have virtually no time to spare anyway so you can do whatever you prefer.

 

[Grey Roger]

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.