I want to help this mod

[olduser]

from Github:
Assert: route cost is 0 #121

Spoiler Github discussion :

alberts2 commented Sep 7, 2019
A zero or negative route movement cost is definitely invalid. Some random events can lead to negative values from getRouteChange.


billw2012 commented Sep 7, 2019
The only way I see to reconcile those two statements is to simply cap the route cost at a minimum of 1. Is that correct?

alberts2 commented Sep 7, 2019
Yes and find out why these random events don't work correctly.

billw2012 commented Sep 7, 2019
Ah I thought you meant the events were intended to do that. If they are not meant to drop getRouteChange below zero, then I shouldn't cap the route cost (thus hiding the problem even though it is still there), but instead leave the assert and fix the underlying bug.

alberts2 commented Sep 7, 2019
For some reason some events get executed multiple times and that leads to things like negative movement costs.

Here's the bts quest that is mentioned

Spoiler :

######## INTERSTATE ###########

def canTriggerInterstate(argsList):

  kTriggeredData = argsList[0]
  trigger = GC.getEventTriggerInfo(kTriggeredData.eTrigger)
  player = GC.getPlayer(kTriggeredData.ePlayer)

  if not player.isCivic(GC.getInfoTypeForString("CIVIC_LIBERAL")):
    return False

  return True

def getHelpInterstate(argsList):
  iEvent = argsList[0]
  kTriggeredData = argsList[1]

  szHelp = TRNSLTR.getText("TXT_KEY_UNIT_MOVEMENT", (1, GC.getRouteInfo(GC.getInfoTypeForString("ROUTE_MODERN_ROAD")).getTextKey()))

  return szHelp

def applyInterstate(argsList):
  iEvent = argsList[0]
  kTriggeredData = argsList[1]
  player = GC.getPlayer(kTriggeredData.ePlayer)
  team = GC.getTeam(player.getTeam())

  iRoad = GC.getInfoTypeForString("ROUTE_MODERN_ROAD")

  team.changeRouteChange(iRoad, -5)

Looks like there is probably no a bug (last line). This quest just needs a tweak.

The bug is that the event is executed multiple times until the movement costs become negative. It should only be executed once.

 

[MattCA]

You don't think -5 route change is large enough to trigger that assert? I'm lookin at the xml now and it's not clear to me without looking further.

Edit: is there a wonder or something that has the same effect? maybe it's a combination of 2 things

 

[Toffer90]

You don't think -5 route change is large enough to trigger that assert? I'm lookin at the xml now and it's not clear to me without looking further.

Edit: is there a wonder or something that has the same effect? maybe it's a combination of 2 things

Only the interstate event and techs can influence the movement stat of MODERN_ROAD. Combined it should only take 30 down to 15.

Maybe the techs are applied more than once, does recalc reset the team route cost modifier (reset values in m_paiRouteChange array) before reapplying the tech modifiers to those route modifiers for the team?
How would then recalc reapply the event modifier if that's the case?

When doing a recalc this is called:
CvTeam::recalculateModifiers()
{

. . .
for(int iI = 0; iI < GC.getNumTechInfos(); iI++)
{

if ( isHasTech((TechTypes)iI) )
{

processTech((TechTypes)iI,1);​

}​

}
. . .​

}
Which leads to this happening:
void CvTeam:: processTech(TechTypes eTech, int iChange, bool bAnnounce)
{

. . .
for (iI = 0; iI < GC.getNumRouteInfos(); iI++)
{

changeRouteChange(((RouteTypes)iI), (GC.getRouteInfo((RouteTypes) iI).getTechMovementChange(eTech) * iChange));
setLastRoundOfValidImprovementCacheUpdate();​

}
. . .​

}
If m_paiRouteChange is reset to an array filled with zeroes when doing recalcs one would expect the modifier from the event to disappear every time a recalc is performed, which would mean that the bug would disappear in saves after doing a recalc if it was the event that was applied multiple times. I don't think the apllyInterstate python code from the interstate event is reprocessed when doing a recalc.

ROUTE_MODERN_ROAD is the only route type in C2C which is modified by techs. So I suspect the bug is caused by doing enough recalcs after one of the modifying techs have been invented.

So either we could fix it by removing the tech modifiers from this route or by modify the recalc to reset the tech modifiers in the m_paiRouteChange array to zero values, but this would mean that a recalc would remove the effect of the interstate event if the event has happened before the recalc. Recalc has always removed stat changes from python though, like how a recalc reset all the era improvements applied to the yield and commerce from the Nasca Lines Wonder.

 

[MattCA]

ha! now there's a bug that's no easy to see. solving it is much easier then finding it.

Bills assert is about something separate actually...
for that bug he's sayin getMovementCost() which is a call to a route info is coming back 0. That value is set using the xml and doesn't change. kinda unsolvable in my mind and its lookin like the mysterious repeating python event rewards bug is solved. It's good to see you getting into the c++ toffer, thanks to bill.

Edit: to make what toffer is saying a bit more clear here's the function that is being called during recalc. (part 3 of the code above)

void CvTeam::changeRouteChange(RouteTypes eIndex, int iChange)
{
    m_paiRouteChange[eIndex] += iChange;
}

 

[MattCA]

its lookin like the mysterious repeating python event rewards bug is solved.

If that explanation is acceptable to alberts of course. I'm just thinkin the are connected. And I was wondering if the values based on xml in the python should be reset for the recalc function. then I start thinkin maybe it should be reset every time a game is loaded. idk

 

[Thunderbrd]

So either we could fix it by removing the tech modifiers from this route or by modify the recalc to reset the tech modifiers (but not other modifiers applied through python events) in the m_paiRouteChange array. We could also reset the entire array to zero, but this would mean that a recalc would remove the effect of the interstate event if the event has happened before the recalc.

I think there's an events record that can be drawn upon for getting modifiers from it after a recalc but I also think this event should just be removed and be reflected by a tech improvement instead. The potential for the event to fire repeatedly and add up to a problem is an issue. It would not be hard to add a tech based modifier after the recalc. I'm surprised it isn't doing it already actually.

It looks like it's made a little complex by caching unfortunately. Not something I can jump on and address right away at the moment.

 

[MattCA]

There is something wrong with the animal spawning code. I am getting ocean critters spawning in fresh water lakes. I suspect we did not make the changes to the system necessary when we introduced lakes as a new terrain.

Looks to me like it's the isFreshWater() function.

Spoiler CvPlot.cpp :

bool CvPlot::isFreshWater(bool bIgnoreJungle) const
{
    CvPlot* pLoopPlot;
    int iDX, iDY;

    if (isWater())
    {
        return false;
    }

    if (isImpassable())
    {
        return false;
    }

    if (isRiver())
    {
        return true;
    }

    for (iDX = -1; iDX <= 1; iDX++)
    {
        for (iDY = -1; iDY <= 1; iDY++)
        {
            pLoopPlot    = plotXY(getX_INLINE(), getY_INLINE(), iDX, iDY);

            if (pLoopPlot != NULL)
            {
                if (pLoopPlot->isLake())
                {
                    return true;
                }

                if (pLoopPlot->getFeatureType() != NO_FEATURE)
                {
                    if (GC.getFeatureInfo(pLoopPlot->getFeatureType()).isAddsFreshWater() && (!bIgnoreJungle || GC.getFeatureInfo(pLoopPlot->getFeatureType()).getHealthPercent() >= 0))
                    {
                        return true;
                    }
                }

                if (pLoopPlot->isCity())
                {
                    CvCity* pCity = pLoopPlot->getPlotCity();
                    if (pCity->hasFreshWater())
                    {
                        return true;
                    }
                }
            }
        }
    }

    return false;
}

You can see when it loops, if an adjacent plot is a lake then freshwater is true but the plot is never checked itself for a lake.
Just throw in if (isLake()) { return true; } first, before isWater() and it should be all good.

 

[Thunderbrd]

Looks to me like it's the isFreshWater() function.

Spoiler CvPlot.cpp :

bool CvPlot::isFreshWater(bool bIgnoreJungle) const
{
    CvPlot* pLoopPlot;
    int iDX, iDY;

    if (isWater())
    {
        return false;
    }

    if (isImpassable())
    {
        return false;
    }

    if (isRiver())
    {
        return true;
    }

    for (iDX = -1; iDX <= 1; iDX++)
    {
        for (iDY = -1; iDY <= 1; iDY++)
        {
            pLoopPlot    = plotXY(getX_INLINE(), getY_INLINE(), iDX, iDY);

            if (pLoopPlot != NULL)
            {
                if (pLoopPlot->isLake())
                {
                    return true;
                }

                if (pLoopPlot->getFeatureType() != NO_FEATURE)
                {
                    if (GC.getFeatureInfo(pLoopPlot->getFeatureType()).isAddsFreshWater() && (!bIgnoreJungle || GC.getFeatureInfo(pLoopPlot->getFeatureType()).getHealthPercent() >= 0))
                    {
                        return true;
                    }
                }

                if (pLoopPlot->isCity())
                {
                    CvCity* pCity = pLoopPlot->getPlotCity();
                    if (pCity->hasFreshWater())
                    {
                        return true;
                    }
                }
            }
        }
    }

    return false;
}

You can see when it loops, if an adjacent plot is a lake then freshwater is true but the plot is never checked itself for a lake.
Just throw in if (isLake()) { return true; } first, before isWater() and it should be all good.

Aren't you on the team with us here? Go for it! Have you gotten setup on the Git stuff yet?

 

[MattCA]

actually I havn't been able to make a github account for some reason. On the create account page it never stops loading and I can't click create account. I'd rather try to solve c2c bugs then that.

my plan has always been to ask to join once I'm done with the first c++ thing I started. I took long 8 month break after a whole load of failure, but I'm gonna get back on it now.

 

[Thunderbrd]

actually I havn't been able to make a github account for some reason. On the create account page it never stops loading and I can't click create account. I'd rather try to solve c2c bugs then that.

my plan has always been to ask to join once I'm done with the first c++ thing I started. I took long 8 month break after a whole load of failure, but I'm gonna get back on it now.

Doing small things like these fixes is a huge help. Seriously, it shouldn't be so hard to sort out the github thing. You may already have an account under your email address so maybe try to use the 'forgot password' option.

 

[Toffer90]

It would not be hard to add a tech based modifier after the recalc. I'm surprised it isn't doing it already actually.

You must have misunderstood my hypothesis then.
My hypothesis is that it is adding the tech modifiers after recalc, and that the bug is that it isn't resetting the modifiers before adding the tech modifiers in the recalc, so each recalc is adding more and more tech modifiers to the route movement...

I think there's an events record that can be drawn upon for getting modifiers from it after a recalc but I also think this event should just be removed and be reflected by a tech improvement instead. The potential for the event to fire repeatedly and add up to a problem is an issue.

I agree that the event should be removed even if it's not actually causing a bug here according to my hypothesis. Events like these, that are making changes to game object stats are all flawed in that a recalc would undermine it.
Like how the Nasca Lines yield and commerce increases applied through python whenever the player reach a new era, those stat increases are removed on each recalc. So if we remove the interstate event, then we should also remove this effect from Nasca Lines so that there are no stat increases for it when reaching a new era. We should also remove the fishing boat yield increase from building that wonder (can't remember its name) that does that through python for the same reasons, as a recalc undermines the wonder effect.

 

[Dancing Hoskuld]

You must have misunderstood my hypothesis then.
My hypothesis is that it is adding the tech modifiers after recalc, and that the bug is that it isn't resetting the modifiers before adding the tech modifiers in the recalc, so each recalc is adding more and more tech modifiers to the route movement...
I agree that the event should be removed even if it's not actually causing a bug here according to my hypothesis. Events like these, that are making changes to game object stats are all flawed in that a recalc would undermine it.
Like how the Nasca Lines yield and commerce increases applied through python whenever the player reach a new era, those stat increases are removed on each recalc. So if we remove the interstate event, then we should also remove this effect from Nasca Lines so that there are no stat increases for it when reaching a new era. We should also remove the fishing boat yield increase from building that wonder (can't remember its name) that does that through python for the same reasons, as a recalc undermines the wonder effect.

Sounds more like the recalc function is bugged to me

 

[Thunderbrd]

My hypothesis is that it is adding the tech modifiers after recalc, and that the bug is that it isn't resetting the modifiers before adding the tech modifiers in the recalc, so each recalc is adding more and more tech modifiers to the route movement...

Then the problem is certainly that the total isn't being zeroed out as is the method underlying recalculation to begin with. Zero out the total and then rebuild to what it should currently be. If we've missed that variable in the zeroing out stage, that's a problem.

Like how the Nasca Lines yield and commerce increases applied through python whenever the player reach a new era, those stat increases are removed on each recalc

I would say that is flat out a problem with the whole method. Those should be tags on the Nasca Lines improvement instead of some python aftereffect so that the recalc can draw on that info, validate whether it should apply, and add it back in after the initial step of zeroing out the final total variable and reconstructing it with all applicable influences thereafter.

We should also remove the fishing boat yield increase from building that wonder (can't remember its name) that does that through python for the same reasons, as a recalc undermines the wonder effect.

Again, should be made a tag instead.

 

[Toffer90]

Sounds more like the recalc function is bugged to me

Yeah, that's my point; I don't think there's any bug where the interstate event happens multiple times for a player.....
Thinking about it again, the interstate event could perhaps happen for two different players who are on the same team, it modifies the route costs for the team, so it would stack for each player in the team that triggers the event. I'm not entirely sure about this though, the question is then if events are trigger on a per team basis, or if they are triggered on a per player basis, this I don't know. An easy fix for this is to set the bGlobal bool for the trigger so that when it happens for one player it will count as having happened for all players, but will only give the route bonus to one of the teams in the game.

If we want python changes to game object stats to survive a recalc, python would need to cache all modification it does in scriptData, and then the dll recalc needs to send python a "recalc finished" event so that python can look through the specific scriptData and reapply any effects that were active in the game before the recalc was performed.

This is a bigger project though.

Again, should be made a tag instead.

I agree, tags are preferable in these cases as it makes the dll aware of the sources of that which makes up the current active stats for things. Which is important for recalc to work.

Though a tag would have a hard time to tell the recalc what random commerce and yield was increased at each of the eras advancements since the nasca lines was built. The dll could use the scriptData for a city to record what building in what city got changed by how much up till this point by the era advancements due to that tag...

 

[Thunderbrd]

cvcity can store a variable specifically to track that but I dont see the value of such a random effect anyhow.

 

[Dancing Hoskuld]

cvcity can store a variable specifically to track that but I dont see the value of such a random effect anyhow.

It allows people to mod the game!

 

[Thunderbrd]

It allows people to mod the game!

I meant, in an unusually short statement since I was on my phone, that I don't get why we would want a random adjustment to the yields of an improvement. Why shouldn't those always be just fixed based on what's happening, like earning a tech level or something along those lines? Adding random moments where a yield calc could change just makes balance work hell.

 

[Dancing Hoskuld]

I meant, in an unusually short statement since I was on my phone, that I don't get why we would want a random adjustment to the yields of an improvement. Why shouldn't those always be just fixed based on what's happening, like earning a tech level or something along those lines? Adding random moments where a yield calc could change just makes balance work hell.

Ah yes the argument that the writer of the code is more important that the user of the code. I have never subscribed to that view.

 

[Toffer90]

I meant, in an unusually short statement since I was on my phone, that I don't get why we would want a random adjustment to the yields of an improvement. Why shouldn't those always be just fixed based on what's happening, like earning a tech level or something along those lines? Adding random moments where a yield calc could change just makes balance work hell.

Clarification:
Nasca Lines is a world wonder, and this one has a creative effect that makes its commerce and yield stronger for each era advanced since it was built, what commerce or yield that increases is random.

 

[Thunderbrd]

what commerce or yield that increases is random.

Yeah, that's what I would urge abandoning and going instead to a mapped out process of improvements with techs (lifestyle ones are there for this by era intention) if the building is owned. That's something tags can be used to plot out. Random effects are problematic for tracking to bugs because you can't know if it did it right or not really.

Ah yes the argument that the writer of the code is more important that the user of the code. I have never subscribed to that view.

Strange take on what I said that shows a decided lack of understanding my statement I think. I have no idea what the connection is between what you're saying and what you're quoting. I don't see any value in such a randomness to an end user either. What would it matter to a player whether we had a specific improvement plan for a wonder's effect vs some randomized improvement plan?