Mod-Modders Guide to Fall Further

[MootPoint]

So I just started looking at the code diffs in patch J, and I'm curious about this new assert marking the memory leak of CvDiploParameters in AI_doPeace(). From what I see in the original BTS code, this is also the way they handle it pretty much everywhere. So either the app leaks everywhere when one of these classes is needed, or else the host is doing the cleanup on the other side (which is really bad design, if that's the case - the lib that executes the allocation should also be responsible for doing the cleanup.)

So, was this assert added because it was a possible problem while hunting for memory leaks? Or is it bad enough that it needs fixing asap? (Has anyone put a trace to figure out how many of these are actually being allocated? Maybe there's not enough of them to be overly concerned about... )

 

[Breez]

Just wanted to state the obvious here...

But Mootpoint knows more than me about stuff

 

[MootPoint]

Question regarding the source code - if I have modifications I'd like to submit back (for instance, slight code refactoring to make things easier to read, like when a function runs off the end of the page), what's the usual procedure?

 

[MagisterCultuum]

I've got this right now, if that's what you mean. I'll have to tone down the damage, as it effects the entire stack, but that's for later.

Edit: Got it checking to see if you're at war. Had an issue when I was testing it, because it had been working when unowned originally. Now, it does not... The moment I put it in a Barb city's borders, it worked. If I had tried that earlier, I'd have had it working right 5 or 6 revisions ago....

def atRangeCitadel(pCaster, pPlot):
    iPlayer = pPlot.getOwner()
    pPlayer = gc.getPlayer(iPlayer)
    eTeam = gc.getTeam(pPlayer.getTeam())
    if pPlayer != pCaster.getOwner():
        if eTeam.isAtWar(pCaster.getTeam()):
            pCaster.doDamageNoCaster(20, 40, gc.getInfoTypeForString('DAMAGE_FIRE'), False)

Technically, the line if pPlayer != pCaster.getOwner(): is completely unnecessary, as there is no way you can be at war with yourself.


Also, I'm pretty sure it wouldn't effect the whole stack, it would effect every enemy unit that moves into an adjacent tile that turn. Units that stay in place would be unharmed.


Adding CyEngine().triggerEffect(gc.getInfoTypeForString(' EFFECT_PILLAR_OF_FIRE'),pPlot.getPoint()) right after pCaster.doDamageNoCaster(20, 40, gc.getInfoTypeForString('DAMAGE_FIRE'), False) should be all you need if you still want the pillar effects.

 

[Valkrionn]

Technically, the line if pPlayer != pCaster.getOwner(): is completely unnecessary, as there is no way you can be at war with yourself.

That one is a holdover from before I got the atWar check working. Removed.

Also, I'm pretty sure it wouldn't effect the whole stack, it would effect every enemy unit that moves into an adjacent tile that turn. Units that stay in place would be unharmed.

That's actually what I meant, all units that move into range are affected. Meaning if an enemy brings a SoD into range, they are all damaged.

Adding CyEngine().triggerEffect(gc.getInfoTypeForString(' EFFECT_PILLAR_OF_FIRE'),pPlot.getPoint()) right after pCaster.doDamageNoCaster(20, 40, gc.getInfoTypeForString('DAMAGE_FIRE'), False) should be all you need if you still want the pillar effects.

Doesn't seem to work...

 

[MagisterCultuum]

Hmm...you don't have the Effects Disabled option on, do you? The game sometimes turns that on on its own, saying your machine isn't up for playing the game with that setting on.


Edit: I just now noticed the PM you sent me about this this morning. May I presume I have already addressed the issue adequately?

 

[xienwolf]

So I just started looking at the code diffs in patch J, and I'm curious about this new assert marking the memory leak of CvDiploParameters in AI_doPeace(). From what I see in the original BTS code, this is also the way they handle it pretty much everywhere. So either the app leaks everywhere when one of these classes is needed, or else the host is doing the cleanup on the other side (which is really bad design, if that's the case - the lib that executes the allocation should also be responsible for doing the cleanup.)

So, was this assert added because it was a possible problem while hunting for memory leaks? Or is it bad enough that it needs fixing asap? (Has anyone put a trace to figure out how many of these are actually being allocated? Maybe there's not enough of them to be overly concerned about... )

I added an Assert everywhere that I found ()] being used (unless to assign array size during 'new'), just as a backup should this come up again in the future and I have forgotten about the lesson learned this time. Almost without doubt that Assert will never fire.

Question regarding the source code - if I have modifications I'd like to submit back (for instance, slight code refactoring to make things easier to read, like when a function runs off the end of the page), what's the usual procedure?

Just post the changes or changed files here and state a reason. All the code exists mentally for me once written, so I tend to aim for compact instead of readable, but have little personal preference about the end aesthetic.

 

[Valkrionn]

On my cell phone atm, so I can't be completely sure but seeing as I can see the effect when the spell itself is cast, I don't think so. And yes, you've covered everything that I had asked. All I've got left is to decide whether or not to put in a check allowing the unit to take damage if it is an enemy, or if the fort is unowned, and getting the effect to work. As it is now it is fine though.

 

[Iceciro]

Current Version Changelog

Patch J
c. Add: CIV4CivilizationInfos tag group: <FeatureHealthPercentChanges>
d. Add: CIV4CivilizationInfos tag group: <TerrainYieldChanges>
e. Add: CIV4CivilizationInfos tag group: <FeatureYieldChanges>

I don't see FeatureYieldChanges as an option, only improvement yield. Does it do features, or improvements? I really want to shift some feature yields (not just health, but hammers/commerce/more) over to civilizations, but it's not in the CivilizationInfos nor the Schema.

 

[Valkrionn]

Yeah, I was sad to see that as well...

 

[Jean Elcard]

I don't see FeatureYieldChanges as an option, only improvement yield. Does it do features, or improvements? I really want to shift some feature yields (not just health, but hammers/commerce/more) over to civilizations, but it's not in the CivilizationInfos nor the Schema.

FeatureYieldChanges was a typo made by me when I was writing down the change log. Really it's ImprovementYieldChanges. FeatureYieldChanges might come with a future release, even if it is just for the sake of completeness. I didn't have a use for it myself, so I didn't add it.

 

[Iceciro]

I can see it in a lot of ways - locking the bonus production from Deep Forests to the lizard civs is one good example, locking the bonus commerce for Haunted Lands to the Scions is yet another...

 

[Tarquelne]

Really it's ImprovementYieldChanges.

Really?

I guess so, you just said.

I've got a use for that...

 

[Iceciro]

Really?

I guess so, you just said.

I've got a use for that...

I wonder if Tarq is considering giving the Remnants of Patria a nice Scion boost like I did *whistles*

 

[Jean Elcard]

Really, it's true, it's ImprovementYieldChanges!

It was a request from Ahwaric for Orbis. After coding it I just wanted to use it myself in FlavourMod (too hard I think now) and decided to give the elven farms a -1 ... not the best idea I ever had ...

 

[Valkrionn]

So, anyone able to tell me why this is refusing to work?

def atRangeCitadel(pCaster, pPlot):
	iPlayer = pPlot.getOwner()
	pPlayer = gc.getPlayer(iPlayer)
	eTeam = gc.getTeam(pPlayer.getTeam())
	if eTeam.isAtWar(pCaster.getTeam()):
		pCaster.doDamageNoCaster(20, 40, gc.getInfoTypeForString('DAMAGE_FIRE'), False)
		CyEngine().triggerEffect(gc.getInfoTypeForString(' EFFECT_PILLAR_OF_FIRE'), (pCaster.getPlot()).getPoint())

It causes the damage just fine, but the effect will not trigger.

 

[xienwolf]

Looks pretty good to me. Maybe it is a subtle typo? Remember what I said about just copying pillar of fire to make your life easier? Could still be your best approach. Posted it below for you, your code is in orange and then everything out of that is a direct copy/paste from citadel of light.

This way you also have a chance of causing fires to start, and will hit neutral units in the same stack as your enemies, but there shouldn't be any chance of declaring war since it is NoCaster type of damage. But it might be best to get rid of the loop over all units even though it IS appropriate that neutral units get hit in the tile, since this code will run once for each and every unit to move into the tile. But that is ALSO an argument to NOT use an effect, because that effect will fire 50 times at once if a stack of 50 enemies moves in range.

[COLOR="Orange"]def atRangeCitadel(pCaster, pPlot):
	iPlayer = pPlot.getOwner()
	pPlayer = gc.getPlayer(iPlayer)
	eTeam = gc.getTeam(pPlayer.getTeam())
	if eTeam.isAtWar(pCaster.getTeam()):
		pBestPlot = pCaster.getPlot()
[/COLOR]
	for i in range(pBestPlot.getNumUnits()):
		pUnit = pBestPlot.getUnit(i)
		pUnit.doDamageNoCaster(20, 40, gc.getInfoTypeForString('DAMAGE_FIRE'), False)
	if (pBestPlot.getFeatureType() == gc.getInfoTypeForString('FEATURE_FOREST') or pBestPlot.getFeatureType() == gc.getInfoTypeForString('FEATURE_JUNGLE')):
		bValid = True
		iImprovement = pPlot.getImprovementType()
		if iImprovement != -1 :
			if gc.getImprovementInfo(iImprovement).isPermanent():
				bValid = False
		if bValid:
			if CyGame().getSorenRandNum(100, "Flames Spread") <= gc.getDefineINT('FLAMES_SPREAD_CHANCE'):
				pBestPlot.setImprovementType(gc.getInfoTypeForString('IMPROVEMENT_SMOKE'))
	CyEngine().triggerEffect(gc.getInfoTypeForString('EFFECT_PILLAR_OF_FIRE'),pBestPlot.getPoint())

 

[Valkrionn]

That actually kills the code . I think the atRange function is just very specific about what will and will not work. I know the for i in range(pBestPlot.getNumUnits()): line does not work with it, but even removing it the code fails to run. Tried with and without it.

Edit: I think I found the problem... It doesn't look like getPlot() is valid for pCaster. pCaster.getPlot() is not used anywhere in CvSpellInterface.

 

[MagisterCultuum]

Ah, it seems you are right. For units, it isn't .getPlot() it is just .plot()

 

[MootPoint]

Some things I've encountered with the latest build:

CvCity.cpp, line 105:

FAssertMsg(m_afProximityCulture==NULL, "About to leak Memory in CvCity::CvCity()");

This assert needs to be removed. There's no leak here, as we're in the constructor; moreover, in debug, this fires all the time for me, as uninitialized pointers come up as 0xbaadf00d by default.


CvPlayerAI.cpp, line 18025, these asserts are currently > 0, they should be >= 0:

FAssert(aiDomainSums[GC.getUnitInfo(eLoopUnit).getDomainType()] >= 0);
FAssertMsg(GC.getUnitInfo(eLoopUnit).getDomainType() >= 0, "Warning, about to leak memory in CvPlayerAI::AI_doEnemyUnitData()");
FAssertMsg(GC.getUnitInfo(eLoopUnit).getUnitClassType() >= 0, "Warning, about to leak memory in CvPlayerAI::AI_doEnemyUnitData()");