Trait_FreePromotionUnit* Rely on LostOnUpgrade = false to function

[LessRekkless]

1. Mod Version (X.Y.Z). Current Version: 3.0.4

3.0.4

4. Describe the Issue

If a promotion with LostOnUpgrade is given to a set of UnitClasses or UnitCombats via Trait_FreePromotionUnitClass or Trait_FreePromotionUnitCombat, that promotion will not exist when the unit upgrades into another UnitClass that qualifies for the promotion in these tables.

This is because the unit gains the free promotion in CvPlayer::initUnit(), but then loses it during CvUnit::convert(). Steps:

CvUnit* CvUnit::DoUpgrade(bool bFree) >
{
  ...
  CvUnit* CvUnit::DoUpgradeTo(UnitTypes eUnitType, bool bFree) >
  {
    ...
    CvUnit* pNewUnit = thisPlayer.initUnit(eUnitType, getX(), getY(), NO_UNITAI, REASON_UPGRADE, false, false, 0, 0, NO_CONTRACT, true, this);
    {
      ...
      pUnit->init(pUnit->GetID(), eUnit, ((eUnitAI == NO_UNITAI) ? pkUnitDef->GetDefaultUnitAIType() : eUnitAI), GetID(), iX, iY, eReason, bNoMove, bSetupGraphical, iMapLayer, iNumGoodyHutsPopped, eContract, bHistoric, pPassUnit); >
      {
        ...
        initWithNameOffset(iID, eUnit, -1, eUnitAI, eOwner, iX, iY, eReason, bNoMove, bSetupGraphical, iMapLayer, iNumGoodyHutsPopped, eContract, bHistoric, false, pPassUnit); >
        {
          ...
          if(getUnitInfo().GetFreePromotions(iI))
          {
            ePromotion = (PromotionTypes) iI;

            PromotionTypes ePromotionRoughTerrain = (PromotionTypes)GC.getInfoTypeForString("PROMOTION_ROUGH_TERRAIN_ENDS_TURN");
            if(ePromotion == ePromotionRoughTerrain && kPlayer.GetPlayerTraits()->IsConquestOfTheWorld())
              continue;

            if(!GC.getPromotionInfo(ePromotion)->IsHoveringUnit() && !GC.getPromotionInfo(ePromotion)->IsConvertUnit()) // Hovering and Convert domain units handled above
              setHasPromotion(ePromotion, true);
          }
          ...
          if(kPlayer.GetPlayerTraits()->HasFreePromotionUnitCombat(promotionID, unitCombatType))
          {
            setHasPromotion(promotionID, true);
          }
          ...
          if(kPlayer.GetPlayerTraits()->HasFreePromotionUnitClass(promotionID, unitClassType))
          {
            setHasPromotion(promotionID, true);
          }
          ...
        } 
      }
    }
    ...
    pNewUnit->convert(this, true); >
    {
      bool bGivePromotion = false;
      ...
      // Old unit has the promotion
      if(pUnit->isHasPromotion(ePromotion) && !pkPromotionInfo->IsLostWithUpgrade())
      {
        bGivePromotion = true;
      }
      // New unit gets promotion for free (as per XML)
      else if(getUnitInfo().GetFreePromotions(ePromotion) && (!bIsUpgrade || !pkPromotionInfo->IsNotWithUpgrade()))
      {
        bFree = true;
        bGivePromotion = true;
      }
      ...
      setHasPromotion(ePromotion, bGivePromotion);
      ...
    }
  }
}

note that it manages to avoid this problem with GetFreePromotions(ePromotion) by having an additional if-statement that catches it.

I guess we just need to add it into the convert step?

[pineappledan]

What is affected by this? Are you trying to give a civ a promotion that is lost on upgrade?

[LessRekkless]

Gwennog would like to do so for one of his modmods.

[LessRekkless]

Also, the Traits_FreePromotions table is non-functional (no dll integration), and while it existed in BNW, was nonfunctional even then. No idea what it would be used for, anyway.

[LessRekkless]

Additionally, we tried giving eg a Spearman a lost in upgrade promotion. Upgrading a Warrior that did not have the promotion to Spearman did not give the Spearman the promotion.

[LessRekkless]

Does anyone see anything wrong with the convert() function just keeping promotions that existed on the unit already (ie the free promotions from the init process), without checking to see if the old unit also has them?

[LessRekkless]

This code also suggests that a captured unit (coercion or price ships) would not gain promotions from policies etc

[gwennog]

Thank you Rekk, I couldn't have posted something so specific.

[gwennog]

Has this issue been fixed in version 3.2?

[RecursiveVision]

I assume not.

[gwennog]

Okay, it is not serious. You have already done so much work in the latest version. Maybe for the next one?

[gwennog]

Hi, Finally ,Is there a chance that the correction proposed by @LessRekkless will be made for the next version? That would be cool.

[azum4roll]

But there may also be a use case where a civ wants a free trait promotion that's NOT kept on upgrade. Which sounds more natural.

Or maybe that should use the NotWithUpgrade column that's weirdly bundled with the promotion and not the free promotion table?

[gwennog]

Or maybe that should use the NotWithUpgrade column that's weirdly bundled with the promotion and not the free promotion table?

Precisely, whether the Promotion column is LostWithUpgade at 0 or 1, the unit does not lose promotion when it is upgraded. If this column worked for me it would solve the problem. And that seems logical, unless I'm mistaken since it's required that the promotion be lost upon upgrade, why isn't it?

[gwennog]

I don't understand, even with lua function, I don't remove the promotion !

`function LoseGranCompanyiaPromotionOnUpgrade(iPlayer, iOldUnit, iNewUnit) local pPlayer = Players[iPlayer] local pOldUnit = pPlayer:GetUnitByID(iOldUnit)

if not pOldUnit:IsHasPromotion(ePromotionGranCompanyiaEM) then return end

local pNewUnit = pPlayer:GetUnitByID(iNewUnit)

if pNewUnit:IsHasPromotion(ePromotionGranCompanyiaEM) then
    pNewUnit:SetHasPromotion(ePromotionGranCompanyiaEM, false)
end

end

if Game.IsCivEverActive(eCivilizationCatalonia) then GameEvents.UnitUpgraded.Add(LoseGranCompanyiaPromotionOnUpgrade) end`

[gwennog]

OK, sorry, to reset the promotion I must put at false the promotion of OldUnit and NewUnit. No sure why but it works.