Something odd is going on

[olduser]

Read that function again. In the merge function, all units are checked to see if they should adopt the AItype of the head unit, not whether the AItype should be changed for the head unit alone. I have a feeling that might be more the intention of whoever designed it though.

I not able to see that? only the new head units ai is changed there to that of the old head units ai. Maybe I should watch it using the debugger.

After looking at this again i still can't see how that function changes every Units AI to that of the head Units AI

void CvSelectionGroup::mergeIntoGroup(CvSelectionGroup* pSelectionGroup)
{
    CvPlayerAI& kPlayer = GET_PLAYER(getOwnerINLINE());

    // merge groups, but make sure we do not change the head unit AI
    // this means that if a new unit is going to become the head, change its AI to match, if possible
    // AI_setUnitAIType removes the unit from the current group (at least currently), so we have to be careful in the loop here
    // so, loop until we have not changed unit AIs
    bool bChangedUnitAI;
    do
    {
        bChangedUnitAI = false;

        // loop over all the units, moving them to the new group,
        // stopping if we had to change a unit AI, because doing so removes that unit from our group, so we have to start over
        CLLNode<IDInfo>* pUnitNode = headUnitNode();
        while (pUnitNode != NULL && !bChangedUnitAI)
        {
            CvUnit* pLoopUnit = ::getUnit(pUnitNode->m_data);
            pUnitNode = nextUnitNode(pUnitNode);
            
            if (pLoopUnit != NULL)
            {
                UnitAITypes eUnitAI = pLoopUnit->AI_getUnitAIType();

                // if the unitAIs are different, and the loop unit has a higher val, then the group unitAI would change
                // change this UnitAI to the old group UnitAI if possible
                CvUnit* pNewHeadUnit = pSelectionGroup->getHeadUnit();
                UnitAITypes eNewHeadUnitAI = pSelectionGroup->getHeadUnitAI();
                if (pNewHeadUnit!= NULL && eUnitAI != eNewHeadUnitAI && pLoopUnit->AI_groupFirstVal() > pNewHeadUnit->AI_groupFirstVal())
                {
                    // non-zero AI_unitValue means that this UnitAI is valid for this unit (that is the check used everywhere)
                    if (kPlayer.AI_unitValue(pLoopUnit->getUnitType(), eNewHeadUnitAI, NULL) > 0)
                    {
                        FAssert(pLoopUnit->AI_getUnitAIType() != UNITAI_HUNTER);
                        // this will remove pLoopUnit from the current group
                        pLoopUnit->AI_setUnitAIType(eNewHeadUnitAI);

                        bChangedUnitAI = true;
                    }
                }

                pLoopUnit->joinGroup(pSelectionGroup);
            }
        }
    }
    while (bChangedUnitAI);
}

It only changes the AI in case another unit becomes the head unit and the old head units AI is valid for the new head unit.. If a unit doesn't become the new head unit no AI Change is made.

Whatever big issue you have been seeing doesn't come from that function. It is valid that the new head unit get's the AI of the old head unit if that AI is a valid AI for the new head unit.

 

[olduser]

Healers and See Invisible types were taking on City Attack AI as soon as joining the stack

@Thunderbrd
CvUnit::joinGroup makes a call to CvSelectionGroup::findNewLeader.

// if we were the head, if the head unitAI changed, then force the group to separate (non-humans)
                if (bWasHead)
                {
                    for (int iI = 0; iI < NUM_UNITAI_TYPES; iI++)
                    {
                        if (isWaitingOnUnitAI(iI))
                        {
                            setToWaitOnUnitAI((UnitAITypes)iI, false);
                        }
                    }
                    FAssert(pOldSelectionGroup->getHeadUnit() != NULL);
                    if (pOldSelectionGroup->getHeadUnit()->AI_getUnitAIType() != AI_getUnitAIType())
                    {
                        //    Special case to try to hold together city attacks that are breaking up but can still suceed
                        //    If we have lost the last city_attack AI unit see if we have a unit that COULD take over in
                        //    the SAME role
                        if ( UNITAI_ATTACK_CITY != AI_getUnitAIType() ||
                             !pOldSelectionGroup->findNewLeader(UNITAI_ATTACK_CITY))
                        {
                            pOldSelectionGroup->AI_makeForceSeparate();
                        }
                    }
                }

Which is the root of your Problems because of the else if ( pLoopUnit->canAttack() )

bool CvSelectionGroup::findNewLeader(UnitAITypes eAIType)
{
    CLLNode<IDInfo>* pUnitNode = headUnitNode();
    CvUnit* pLoopUnit;
    CvUnit* pBestUnit = NULL;

    while (pUnitNode != NULL)
    {
        pLoopUnit = ::getUnit(pUnitNode->m_data);
        pUnitNode = nextUnitNode(pUnitNode);

        if ( GC.getUnitInfo(pLoopUnit->getUnitType()).getUnitAIType(eAIType) )
        {
            pBestUnit = pLoopUnit;
            break;
        }
        else if ( pLoopUnit->canAttack() )
        {
            //    If there is no unit that explicitly supports the attack city role
            //    accept any that can attack so as to keep the stack's intent
            pBestUnit = pLoopUnit;
        }
    }

    if ( pBestUnit != NULL )
    {
        pBestUnit->AI_setUnitAIType(eAIType);
        pBestUnit->joinGroup(this);
        return true;
    }

    return false;
}

Try this instead and the things should get better.

bool CvSelectionGroup::findNewLeader(UnitAITypes eAIType)
{
    CLLNode<IDInfo>* pUnitNode = headUnitNode();
    CvUnit* pLoopUnit;
    CvUnit* pBestUnit = NULL;
    CvPlayer& kPlayer = GET_PLAYER(getOwnerINLINE());

    while (pUnitNode != NULL)
    {
        pLoopUnit = ::getUnit(pUnitNode->m_data);
        pUnitNode = nextUnitNode(pUnitNode);

        if (kPlayer.AI_unitValue(pLoopUnit->getUnitType(), eAIType, NULL) > 0)
        {
            pBestUnit = pLoopUnit;
            break;
        }
    }

    if ( pBestUnit != NULL )
    {
        pBestUnit->AI_setUnitAIType(eAIType);
        pBestUnit->joinGroup(this);
        return true;
    }

    return false;
}

 

[Toffer90]

You should listen to alberts TB, he's a code wizard. ^^

 

[Thunderbrd]

Ok, so under closer evaluation, looks like I read this opposite ( with a < )
pLoopUnit->AI_groupFirstVal() > pNewHeadUnit->AI_groupFirstVal()

Once seeing it the way I thought I had come to understand it, the symptoms seemed to have an obvious answer.

So you're saying that just removing

       else if ( pLoopUnit->canAttack() )
        {
            //    If there is no unit that explicitly supports the attack city role
            //    accept any that can attack so as to keep the stack's intent
            pBestUnit = pLoopUnit;
        }

should do the trick. It also looks like I need to make SURE that a few AI types are rated low enough to be subservient to the groups they would normally go to join.

 

[olduser]

So you're saying that just removing

And using AI_unitValue there.
You could also copy the whole changed function from above.

 

[Thunderbrd]

And using AI_unitValue there.
You could also copy the whole changed function from above.

Which is what I did. My first attempt to visually compare was on my phone with about 2 min to spare so I was a bit challenged to see the differences specifically.

 

[olduser]

There's another possible issue at that spot i have to investigate this later.

 

[Thunderbrd]

There's another possible issue at that spot i have to investigate this later.

By all means. I don't mean to make this discussion sound like I don't appreciate your review. If anything, I was mostly trying to get you to point at what the problem was if it wasn't what I thought I'd found.

 

[Septimius]

No thanks. I rather wait the official patches.

Well, expect to wait a while then. Everybody that plays this game are basically helping the mods iron out the bugs.

 

[Thunderbrd]

There's another possible issue at that spot i have to investigate this later.

I'm not sure what symptoms you are seeing that makes you say this specifically (not pretending there aren't symptoms of strange AI behaviors) but I think I found the underlying flaw that's making it so possible for units to join groups they shouldn't be joining and so on.

The broker call was not demanding units that ARE a particular AI, so much as allowing any units in the field that MAY have the specified AI to respond. When those units would come to answer the call and join the group, they wouldn't change their AIs to the type requested to fulfill the role (which is probably good for maintaining the integrity of unit promotion selection and intentions) so would just end up being chaffe in the stack as the role continues to go unfulfilled to the head AI's sensors and he'd continue calling for the AI type he thinks he needs. There would be other failures in the AI as a result of having these incorrectly responding units go missing from their intended roles as well.

This is kinda related to this issue but was another factor entirely. By making the broker call only be answerable by a unit WITH the specified AI type, we should be seeing a lot better behaviors now. (Once committed at least.)

Is that along the lines of the symptoms of error that you were seeing?