I'm a pessimist by nature, but sometime optimism is more constructive than pessimism. Let's think forward and not backwards in terms of code development, if a fix create a bigger bug than what it fixed then let's look to how we can fix the bigger bug instead of reimplementing the old smaller bugs to avoid the new bigger bug.
Something odd is going on
- Thread starter retired
- Start date
The brokerage system seems to fail because it requests a unitai but the newly trained units answering to the request don't have that unitai defined as valid unitai. So it works as designed.Settlers aren't requested using the brokerage system but somehow scouts are chosen as best unit for that role.
The evaluation of which unit is the best for a unitai is completely broken it seems.
The NotUnitAIs tag stops the AI from upgrading, thats something i changed a while ago. Units keep their unitai on upgrade even if it isn't valid at least this is how it used to be.
Thunderbrd
C2C War Dog
So you're saying that pretty much any old unit is being chosen to answer any old brokerage call? Even if a unitAI is specified in the criteria of the call? That seems to NOT be
Surely the whole point of including an AI designation in a brokerage call is intended to only be answered by units with that AI setting?!? I'll have to look at that much more carefully.
So that is to suggest that it is a more fundamental problem with both brokerage and direct training pushes, or possibly a problem where a unit AI type changes during play. Which then makes me think I've just corrected the largest issue, because the point I was working with appears, if I'm reading the flow correctly and following the debug run process accurately, to lead through the same function where non-AI types are filtered.
int CvPlayerAI::AI_unitValue(UnitTypes eUnit, UnitAITypes eUnitAI, CvArea* pArea, CvUnitSelectionCriteria* criteria) const
The strange thing is that there are other things being used to filter out particular unit types there, such as demanding that Settler units are defined first and foremost by their ability to found a city, and if they can't do that then they can't be selected for that role. So a case like the scout ending up as a settler can hardly be answered by this since a scout cannot found a city. How we're getting around that then makes me suspicious that it's not how or where the unit is called to be generated, but rather that perhaps a scout had joined with a settler unit and taken on the AI of the settler unit when it joined the stack, which would just be a continuation of a bug I had mostly solved already but needed to be more sweeping about the solution. There's a very good chance we've been having a lot of these strange behaviors all along and they're actually less frequent now than they were and I've just failed to patch all the places where these problems begin.
This suggests that there remain larger problems with void CvSelectionGroup::mergeIntoGroup(CvSelectionGroup* pSelectionGroup) where you have:
Code:
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);pLoopUnit->AI_setUnitAIType(eNewHeadUnitAI);
is causing mass havoc because many unit AI types should never change even if they are called to join a group. The checks to see if there enough of those types in the stack then get completely thrown off and find none of those types because their AI changed when joining the stack. Thus we get endless spams of healers and spotters and so on. I'm sure that's supposed to work grand for some things merging into an attack stack or something, though I'm hard pressed to see how that's a good thing there or anywhere else. Units should always know what their role is even if they are bringing that role to a larger collaborative effort. I'm sure this is largely a big part of our city attack stack design problems in particular.
I'm really considering going through and just eliminating this function's usage entirely until it can be made clear why it would ever be useful. joinGroup(pJoinUnit->getGroup()); seems to be a much better solution.
If this has to do with making sure the lead unit is using the best AI type and if it dies, switching the next best unit for that role over, then it needs to be more carefully crafted to ensure that the stack can even do that job after the lead unit is gone - for example, a scout escorting a settler would not want to suddenly be made a settler unit by AI just because the settling took place and the stack's head unit has now changed. But from what I was seeing in-game was that this function, MergeIntoGroup, was causing extremely poor AI changes to units that should never have changed their AI types.
No thanks. I rather wait the official patches.
Thunderbrd
C2C War Dog
Ok, this is resolved on the next commit. Had a problem with units that shouldn't qualify still getting some value - didn't control those calculations well enough. Was a simple bug.
@alberts2 I thought this was the case and I wasn't going crazy... there always have been AI types that can just pick the best unit even if that unit doesn't have that AI type specified. It wasn't new. I had actually included MORE control on that so this was a frustrating bug. However, it was not controlling the iValue on the best unit calculation well enough that caused this. I was allowing the existence of XP and promotion potential to screw up what should've remained a 0 - I made an erroneous change to the manner in which potential XP is tallied into the value of the unit. Stupid mistake... sorry.
@strategyonly
My apologies sir but we're definitely going to need to issue a v38.2 on this. This is an unacceptable bug for a release version. Once I get this on the SVN anyhow. The tech cost issue has also been patched up and should be looking pretty good. We should wait a day or two for emergency bug reports on this revision number but if we don't get any complaints then we should be preparing to package a new download with the fix. This one's important enough.
@alberts2 I thought this was the case and I wasn't going crazy... there always have been AI types that can just pick the best unit even if that unit doesn't have that AI type specified. It wasn't new. I had actually included MORE control on that so this was a frustrating bug. However, it was not controlling the iValue on the best unit calculation well enough that caused this. I was allowing the existence of XP and promotion potential to screw up what should've remained a 0 - I made an erroneous change to the manner in which potential XP is tallied into the value of the unit. Stupid mistake... sorry.
@strategyonly
My apologies sir but we're definitely going to need to issue a v38.2 on this. This is an unacceptable bug for a release version. Once I get this on the SVN anyhow. The tech cost issue has also been patched up and should be looking pretty good. We should wait a day or two for emergency bug reports on this revision number but if we don't get any complaints then we should be preparing to package a new download with the fix. This one's important enough.
I'll stop making python changes untill v38.2 is released. They are too risky, I hope the changes I've already committed are sound.
Thunderbrd
C2C War Dog
Thank you. Been a little worried about that.
Be carefull there.
CvSelectionGroup::mergeIntoGroup is only called in CvUnitAI::AI_groupMergeRange and it never happens unconditionally. Changing unit ai's isn't done unconditionally either, there are situations where it might have to happen.
If you look at CvSelectionGroup::mergeIntoGroup you see a call to CvPlayerAI::AI_unitValue and that is one spot that was changed recently. It seems those recent changes trigger AI changes then they shouldn't happen and didn't happen before these changes where made.
If you look at this
Code:
void CvUnitAI::AI_pillageMove()
{
/************************************************************************************************/
/* BETTER_BTS_AI_MOD 03/05/10 jdog5000 */
/* */
/* Unit AI */
/************************************************************************************************/
PROFILE_FUNC();
if (AI_selectStatus(true))
{
return;
}
//if (AI_guardCity(false, true, 1))
//{
// return;
//}
if (AI_heal(30, 1))
{
return;
}
// BBAI TODO: Shadow ATTACK_CITY stacks and pillage
//join any city attacks in progress
if ( MISSIONAI_REGROUP == getGroup()->AI_getMissionAIType() || (plot()->isOwned() && plot()->getOwnerINLINE() != getOwnerINLINE()))
{
if (AI_groupMergeRange(UNITAI_ATTACK_CITY, 1, false, true))
{
return;
}
}
if (AI_cityAttack(1, 55))
{
return;
}This might be one spot that caused the bugs you have been trying to fix. Units with pillage Ai merging with City attackers.
Thunderbrd
C2C War Dog
The changes were made on the same revision. It has been ages that mergeintogroup has been hounding us with horrendously buggy behavior that has paralyzed the AI's ability to generate attack stacks that it is then willing to utilize. For years we've wondered why rams and healers have been overbuilding, why the attack stacks won't mobilize, and why lately LE units have been spammed to become City Attack units. It's ALL due to this merge into group function. I watched as it kept changing units to AI's they should never be and the use of that valuation, CvPlayerAI::AI_unitValue, does not even attempt to balance one type against another but rather what is more valuable for a unit to be for an AI type in terms of abilities. It is hopeless at trying to balance one AI type vs another. If we want something along those lines then it must be considered for only that purpose. This function also works into picking the best type of unit for a particular AI role. You cannot have a function attempting to not only judge the best apple-like fruit from the best orange-like fruit, but also judge the best overall fruit. Particularly when the concept of a 'best' fruit depends entirely on the situation and purpose of choosing which is best.
Plus, the random chaos (or at least extremely difficult to predict without calculating things with incredible care) introduced by such AI change potentials just because a unit joins a group is really upsetting to any efforts to plan the manner in which unit AI's will work to perform a role within the group of another AI type.
Yeah, that's a good spot to look into. Will have to look at AI_groupMergeRange to make sure it's not just a call to 'join' rather than 'merge'. We might have to start auditing for uses of 'merge' vs uses of 'join' throughout the AI code. It's the sort of thing we gotta be careful about or before you know it you have units running around doing things they were never intended to do and thus doing it extremely poorly and promoting in all the wrong ways.
Last edited:
Thunderbrd
C2C War Dog
So looking at this function closer:
it looks like
'if (kPlayer.AI_unitValue(pLoopUnit->getUnitType(), eNewHeadUnitAI, NULL) > 0)' is being used just to validate the possibility that this unit may be used for the selection group's current AI role. Whether valid or not is a reasonable check to make and this is a reasonable function to do it.
But 'pLoopUnit->AI_groupFirstVal() > pNewHeadUnit->AI_groupFirstVal()' is the main check to see if the unit should change AI types to that of the current group head. That makes a litle more sense but what doesn't make sense is why we want the whole group having the same AI type thus eroding the integrity of the role of each unit individually. Does it make sense to continue to utilize this feature at all?
Code:
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);
}'if (kPlayer.AI_unitValue(pLoopUnit->getUnitType(), eNewHeadUnitAI, NULL) > 0)' is being used just to validate the possibility that this unit may be used for the selection group's current AI role. Whether valid or not is a reasonable check to make and this is a reasonable function to do it.
But 'pLoopUnit->AI_groupFirstVal() > pNewHeadUnit->AI_groupFirstVal()' is the main check to see if the unit should change AI types to that of the current group head. That makes a litle more sense but what doesn't make sense is why we want the whole group having the same AI type thus eroding the integrity of the role of each unit individually. Does it make sense to continue to utilize this feature at all?
Sure great just take your time. I want to remind though that the recent build cost revision is not very balanced and quite illogical at some places like better version of a building costing less than the previous one. While the revision is good in general, several low end buildings are too costly, creating nasty bottlenecks in production and causes the need for player to keep pampering and visiting cities often to buy buildings in attempt to get all the basics in there before the crime and disease spiral out of control.
AI_groupFirstVal is used to define which unitai should be the new head unit.
A hunter has a higher value as a subdued animal so the hunter will always be the head. If the hunter dies the subdued animal becomes the head but a zero AI_unitValue in that case means the AI of the animal doesn't change. The AI_unitValue used to always return zero in case the old head unitai wasn't valid for a unit but that was changed recently. That's one reason why we saw so many units with the wrong unitai.
That functionality seems ok to me and did it's job where does this create trouble? And sometimes issues could arise from wrong UnitAIs and NotUnitAIs setup in the xml.
Thunderbrd
C2C War Dog
When its used to determine what unit should be the head unit, it's probably working pretty well - and I'm finding some examples of how the merge consideration is supposed to work with this, which is different - it's asking if when your units merge with a group if they should take on the AI of the head unit rather than maintaining their own. There's been a lot of problems here from those kinds of AI changes when merging into a group, particularly when a unit is answering a brokerage call. But I'm trying hard to take the time to consider all aspects of the issue instead of just trying to fix those problems by fishing with dynamite, which is what I know I should try to stop veering towards doing but tend to due to the immediate unacceptable nature of the problem I'm working on often. With a little more patience I think I can figure out how to get this all working more effectively for all situations and give us a little more control to say how we want things to perform. I've seen some examples where it's supposed to work in the manner that makes it problematic elsewhere. It just needs a little sorting out is all.
I still don't know what exact situation you tried to fix then you started with AI changes after V38.
If a contract gets fulfiled CvUnit::joinGroup is used so no merging takes place here because only a single unit joins a Group.
In CvUnit::joinGroup you find this.
This means there is a AI Change possible but only in a very special case.
CvSelectionGroup::findNewLeader also checks if UNITAI_ATTACK_CITY is a valid unitai before chosing a new leader.
I can't see how units could just change their ai then they answer a contract.
If you look at all usages of AI_setUnitAIType there are lots of them there no check is made to see if the new unitai is valid for the unit. I would look at those first and if those don't give the answer look at the XML.
I still think all units should have a proper unitai setup in their XML and there should be no exeptions like allowing units to change into other unitai as they have defined.
Edit:
There are a few spots like this one, the ai is just changed AI_setUnitAIType(UNITAI_PILLAGE) without any check if that unitai is valid.
It's possible to call CvUnitAI::AI_pillageMove() here instead of changing the unitai.
If a contract gets fulfiled CvUnit::joinGroup is used so no merging takes place here because only a single unit joins a Group.
In CvUnit::joinGroup you find this.
Code:
// 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();
}
}
}This means there is a AI Change possible but only in a very special case.
CvSelectionGroup::findNewLeader also checks if UNITAI_ATTACK_CITY is a valid unitai before chosing a new leader.
I can't see how units could just change their ai then they answer a contract.
If you look at all usages of AI_setUnitAIType there are lots of them there no check is made to see if the new unitai is valid for the unit. I would look at those first and if those don't give the answer look at the XML.
I still think all units should have a proper unitai setup in their XML and there should be no exeptions like allowing units to change into other unitai as they have defined.
Edit:
There are a few spots like this one, the ai is just changed AI_setUnitAIType(UNITAI_PILLAGE) without any check if that unitai is valid.
Code:
void CvUnitAI::AI_InvestigatorMove()
{
PROFILE_FUNC();
if (AI_selectStatus(true))
{
return;
}
if (AI_isNegativePropertyUnit())
{
AI_setUnitAIType(UNITAI_PILLAGE);
return;
}It's possible to call CvUnitAI::AI_pillageMove() here instead of changing the unitai.
Last edited:
Thunderbrd
C2C War Dog
Fulfilled contracts were using CvSelectionGroup::mergeIntoGroup.
If you look at my post above that shows that function, it's not actually only changing AI types on the joining units when the head unit changes - it's having them all change their AI to the head unit's AI, which has been causing a whole range of bad behaviors. Other 'merge into group' references may suffer from the same flaws as you pointed out.
Later tonight I can look up in the changelog where the mergeintogroup call was being made as part of answering to a contract.
Thunderbrd
C2C War Dog
As promised:
in processContracts(rev9913), where units that are answering a contract are joining the unit that put out the brokerage call, you have the command to join being initiated if they are on the same plot finally:
Code:
if (atPlot(pTargetPlot))
{
m_contractualState = CONTRACTUAL_STATE_FOUND_WORK;
if ( pJoinUnit != NULL && atPlot(pJoinUnit->plot()) )
{
if( gUnitLogLevel >= 3 )
{
logBBAI(" ...already at target plot - merging into requesting unit's group.");
}
getGroup()->setTransportUnit(NULL);
getGroup()->mergeIntoGroup(pJoinUnit->getGroup());
// Remove ourselves from advertising for work and set our status back to no contract
contractFulfilled();
}
else
{
if( gUnitLogLevel >= 3 )
{
logBBAI(" ...already at target plot.");
}
contractFulfilled();
getGroup()->pushMission(MISSION_SKIP);//problem for tracking units fulfilling certain jobs by simply being there. MissionAI is not set this way. Might have to improve on this.
}
}9914 - my adjustment:
Code:
if (atPlot(pTargetPlot))
{
m_contractualState = CONTRACTUAL_STATE_FOUND_WORK;
if ( pJoinUnit != NULL && atPlot(pJoinUnit->plot()) )
{
if( gUnitLogLevel >= 3 )
{
logBBAI(" ...already at target plot - merging into requesting unit's group.");
}
getGroup()->setTransportUnit(NULL);
if (!bNeverChangeAI)
{
getGroup()->mergeIntoGroup(pJoinUnit->getGroup());
}
else
{
joinGroup(pJoinUnit->getGroup());
}
// Remove ourselves from advertising for work and set our status back to no contract
contractFulfilled();
}
else
{
if( gUnitLogLevel >= 3 )
{
logBBAI(" ...already at target plot.");
}
contractFulfilled();
getGroup()->pushMission(MISSION_SKIP);//problem for tracking units fulfilling certain jobs by simply being there. MissionAI is not set this way. Might have to improve on this.
}
}However, I must admit that you give a good example of when it SHOULD be used (or should it? It doesn't really benefit either the hunter or the animal to switch to the other AI type...)
Code:
if ( !isHuman() && plot()->getOwnerINLINE() != getOwnerINLINE() )
{
if ( AI_groupMergeRange(UNITAI_SUBDUED_ANIMAL, 0, false, true, true) )
{
return;
}
// In the worker case we don't want to get sucked into a stack with an already protected
// worker, or one not also in non-owned territory so just search this plot
if ( AI_group(UNITAI_WORKER, 1, -1, -1, false, false, false, 0) )
{
return;
}
}They used to never switch their ai in mergeIntoGroup here. Because the subdued animal ai wasn't valid for the hunter and the hunter ai wasn't valid for the animal it wasn't possible for them to switch. The only exception could have been if a subdued animal has the hunter ai defined on its xml but that isn't a dll coding problem.
Only the new head units ai is changed to that of the old one in mergeIntoGroup but only if that ai was valid for the new head unit.
You changed the behavior of AI_unitValue and that changed the behavior of mergeIntoGroup as well and this made more ai switching possible.
I still can't see how this could have caused lots of trouble.
Thunderbrd
C2C War Dog
Ok, so that makes it reasonable that it still works there without issues - though it's a bit like plotting out a path through a mine field. But is there any reason that they should be even checking?
Are you aware of any situation where all units in the selection group should just become the same AI as the stack head when they merge into the selection group?
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.
The correction has already been made there so it's not currently a problem like it was. All AI_unitValue was doing was validating the possibility of a change by checking to see if the unit being checked also COULD be that unitAI of the head unit. But it was not determining if it was the best of the two (its own or the head unit) as a leader. If it does find that it's not the best of the two then the unit would take the AI of the head unit. I realize that this means that AI_unitValue corrections are now keeping some units from switching to the AI type of the head unit AI, but it does not and cannot control all situations so that you can simply assume that this kind of behavior won't happen during a merge. For example, if a unit CAN be a City Attack AI but is instead a Counter AI and the stack wants X amount of Counter AI types to make up a certain percentage of the stack roughly, then when the stack calls for counter AI types to be made and come merge into the stack then when those units are made and answer the call they become a City Attack AI in the process of joining the stack, then the stack infinitely calls for counter AI types because it never feels like it gets what it's ordered because what it ordered becomes something else on arrival. This sort of tail chasing was happening in a lot of situations before rev9914, paralyzing the growing city attack stack. Healers and See Invisible types were taking on City Attack AI as soon as joining the stack.
Since then, AI_unitValue is far MORE restrictive than it was, invalidating MOST units if they don't specify that they have that unitAI as part of their XML stated options, but there's going to be problems if this isn't changed because I plan more AI types that perform different roles for the stack that can even be assigned to units that can also be City Attack AIs quite validly. And at the same time these would NOT be the kinds of AI you'd want taking over a large stack either, which is what AI_groupFirstVal() is supposed to help determine (which unit, by EXISTING unitAI, should take over the stack if the leader dies or the stack merges - not which AIs should all be adopted by the units in the stack.)
Maintaining the integrity of the AI type even when the unit is part of a larger selectiongroup, is very important.
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.
