getPatrolNode in SavedBattleGame may not be selecting preferred node as intended

[CrazedHarpooner]

While code diving to get some more insight of node destination selection made by AI controlled units, I came accross this piece of code in OXC
https://github.com/OpenXcom/OpenXcom/blob/d8800efdcced960371e68af0659333fc9339910b/src/Savegame/SavedBattleGame.cpp#L1325

Code: [Select]

if (!preferred
|| (unit->getRankInt() >=0 &&
preferred->getRank() == Node::nodeRank[unit->getRankInt()][0] &&
preferred->getFlags() < n->getFlags())
|| preferred->getFlags() < n->getFlags())
But since preferred->getFlags() < n->getFlags()) appears in both in inner AND and the outer OR, the entire AND check is rendered pointless and can be reduced to simply:

Code: [Select]

if (!preferred || preferred->getFlags() < n->getFlags())
Diving deeper I found this commit
https://github.com/OpenXcom/OpenXcom/pull/406/commits/add3873f268cda8ea44cd1b75829f8fdacdb5dea#diff-b82c31883ae959f77ec9d48bc1357827a3d6d67fe74fae84080cdde22a5b6711L1119
And judging by the description, it was meant to make the AI favor nodes that had their matching rank (or their most preferred rank). I am not clear if this meant that it would pick said node over another that had a different rank even if it had a higher 'flags' (patrol priority) or not, but regardless of intent the behaviour with the current if condition effectively ignores the node rank and picks purely on the flags. Unless I'm horribly missing something.

To verify this I turned the condition into logic algebra as so
A = !preferred (yes, with the not included, this could confuse people, but I read it as: 'preferred not set')
B = unit->getRankInt() >= 0 (I read it as: "unit has alien rank" and I understand it is there as a safety short-circuit to prevent segmentation faults with the next part)
C = preferred->getRank() == Node::nodeRank[unit->getRankInt()][0] (Check if the current preferred node has the rank the unit wants the most)
D = preferred->getFlags() < n->getFlags() (Check if node 'n' has better flags than the current preferred node)
Turning the if into "A or (B and C and D) or D" and you should be able to see that this would fall under the absorption law A+(A⋅B)=A in this case D would absorb (B and C and D) resulting in "A or D"

Even if I were to adjust the last or component as such

Code: [Select]

if (!preferred
|| (unit->getRankInt() >=0 &&
preferred->getRank() == Node::nodeRank[unit->getRankInt()][0] &&
preferred->getFlags() < n->getFlags())
|| (unit->getRankInt() < 0 &&
preferred->getFlags() < n->getFlags()))
So it only checks that last OR for units with negative alien rank (X-com units under alien MC by what I've come to understand have this value) the previous rank check still doesn't behave correctly the way I understand it and I'll provide an example:
We have a leader (wants rank 4 nodes the most) based on this https://github.com/OpenXcom/OpenXcom/blob/d8800efdcced960371e68af0659333fc9339910b/src/Savegame/Node.cpp#L59
we iterate through the 1st node and as such the initial preferred check just passes us into the body without further checks, turns out that this 1st node has a rank of 4 and flags of 1, now we iterate through the next node in line and it has a rank of 3 with flags of 2, so we have to check the unit's rank (leader being 1) we check that our CURRENT preferred node matches our wanted rank and that the flags of node 'n' is higher so we proceed to update the preference to this new node even tho it doesn't have our wanted rank (is this the desired outcome?) but now we iterate through a 3rd node, this one has the rank of 4 and flags of 3, when whe perform the rank check, since our current preferred node has a rank of 3, it is different to the units desired rank and even tho this interation of 'n' has higher flags we don't update the preference (this one surely can't be desired). Hope that wasn't too confusing.

Anyway the changes I've come up with in the following code do the following: the unit will prioritize nodes that have its desired rank value over flags, if more than one node has the desired value, then flags will determine which one it will go to, if none of the nodes have its desired rank or the unit has a negative rank it will pick based on flags. It does this by also checking the rank of node 'n' and not only the rank of the currently preferred node.

Code: [Select]

if (!preferred // Preferred node not set (null)
|| (unit->getRankInt() >=0 && // I have an alien rank
((n->getRank() == Node::nodeRank[unit->getRankInt()][0] && (preferred->getRank() != Node::nodeRank[unit->getRankInt()][0] || preferred->getFlags() < n->getFlags())) // AND node(n) Rank has my desired rank AND (preferred node doesn't have my desired rank OR preferred node has lower flags)
|| (preferred->getRank() != Node::nodeRank[unit->getRankInt()][0] && preferred->getFlags() < n->getFlags()))) // OR preferred node doesn't have my desired rank AND it has lower flags
|| (unit->getRankInt() < 0 && // I don't have an alien rank
preferred->getFlags() < n->getFlags())) // AND preferred has lower flags