Code Irregularity

[God-Emperor]

So I'm going through and applying some updates to R2R and when doing an update (AI Andy's code cleanup from rev 4934) I notice something...

Is it just me, or does the code in the "if" at the end of this function do absolutely nothing?

void CvAnimationPathInfo::copyNonDefaults(CvAnimationPathInfo* pClassInfo, CvXMLLoadUtility* pXML)
{
	bool bDefault = false;
	int iDefault = 0;
	int iTextDefault = -1;  //all integers which are TEXT_KEYS in the xml are -1 by default
	int iAudioDefault = -1;  //all audio is default -1	
	float fDefault = 0.0f;
	CvString cDefault = CvString::format("").GetCString();
	CvWString wDefault = CvWString::format(L"").GetCString();

	CvInfoBase::copyNonDefaults(pClassInfo, pXML);

	if (isMissionPath() == bDefault) m_bMissionPath = pClassInfo->isMissionPath();

	int		iCurrentCategory;
	float	fParameter;

	if ( m_vctPathDefinition.empty()) 
	{
		for ( int i = 0; i < (int)m_vctPathDefinition.size(); i++ )
		{
			iCurrentCategory = pClassInfo->getPathCategory(i);
			fParameter = pClassInfo->getPathParameter(i);
			m_vctPathDefinition.push_back( std::make_pair(iCurrentCategory, fParameter ));
		}
	}	
}

If m_vctPathDefinition is empty, the loop should never do anything because it's size should be 0. This was actually more obvious before AI Andy's code cleanup when the condition in the "if" was "m_vctPathDefinition.size() == 0". If the size is zero, loop over all 0 elements. Yeah, right.

It seems to me that it should be calling the getNumPathDefinitions() method of pClassInfo to get the end condition for the loop. I think.

The question is if it isn't working why is nothing in the animations messed up?

The answer to that is that this entire function is never called (possibly because it doesn't work), at least that's what the Visual Studio "find all references" says and the read function immediately before it for the same class certainly doesn't. If you remove it (and the matching reference in the .h file) the DLL builds just fine. So I think the correct "code cleanup" would be to just remove the entire thing, not change one line that makes unused code that did nothing still do nothing.

And that is one reason the R2R updates take so long for me to do: I get sidetracked from time to time. And now I'm wondering how many of the many other similar functions that were added are not actually used. The very next one in the file, CvAnimationCategoryInfo::copyNonDefaults, is also apparently never used either...

 

[Koshling]

So I'm going through and applying some updates to R2R and when doing an update (AI Andy's code cleanup from rev 4934) I notice something...

Is it just me, or does the code in the "if" at the end of this function do absolutely nothing?

void CvAnimationPathInfo::copyNonDefaults(CvAnimationPathInfo* pClassInfo, CvXMLLoadUtility* pXML)
{
	bool bDefault = false;
	int iDefault = 0;
	int iTextDefault = -1;  //all integers which are TEXT_KEYS in the xml are -1 by default
	int iAudioDefault = -1;  //all audio is default -1	
	float fDefault = 0.0f;
	CvString cDefault = CvString::format("").GetCString();
	CvWString wDefault = CvWString::format(L"").GetCString();

	CvInfoBase::copyNonDefaults(pClassInfo, pXML);

	if (isMissionPath() == bDefault) m_bMissionPath = pClassInfo->isMissionPath();

	int		iCurrentCategory;
	float	fParameter;

	if ( m_vctPathDefinition.empty()) 
	{
		for ( int i = 0; i < (int)m_vctPathDefinition.size(); i++ )
		{
			iCurrentCategory = pClassInfo->getPathCategory(i);
			fParameter = pClassInfo->getPathParameter(i);
			m_vctPathDefinition.push_back( std::make_pair(iCurrentCategory, fParameter ));
		}
	}	
}

If m_vctPathDefinition is empty, the loop should never do anything because it's size should be 0. This was actually more obvious before AI Andy's code cleanup when the condition in the "if" was "m_vctPathDefinition.size() == 0". If the size is zero, loop over all 0 elements. Yeah, right.

It seems to me that it should be calling the getNumPathDefinitions() method of pClassInfo to get the end condition for the loop. I think.

The question is if it isn't working why is nothing in the animations messed up?

The answer to that is that this entire function is never called (possibly because it doesn't work), at least that's what the Visual Studio "find all references" says and the read function immediately before it for the same class certainly doesn't. If you remove it (and the matching reference in the .h file) the DLL builds just fine. So I think the correct "code cleanup" would be to just remove the entire thing, not change one line that makes unused code that did nothing still do nothing.

And that is one reason the R2R updates take so long for me to do: I get sidetracked from time to time. And now I'm wondering how many of the many other similar functions that were added are not actually used. The very next one in the file, CvAnimationCategoryInfo::copyNonDefaults, is also apparently never used either...

1) Looks like a bug, as you say
2) CopyNonDefaults() only matters if there is a modular override of the class concerned - we probably never have modular overrides of animations, so you're not seeing any impact of the bug

 

[AIAndy]

So I'm going through and applying some updates to R2R and when doing an update (AI Andy's code cleanup from rev 4934) I notice something...

Is it just me, or does the code in the "if" at the end of this function do absolutely nothing?

If m_vctPathDefinition is empty, the loop should never do anything because it's size should be 0. This was actually more obvious before AI Andy's code cleanup when the condition in the "if" was "m_vctPathDefinition.size() == 0". If the size is zero, loop over all 0 elements. Yeah, right.

It seems to me that it should be calling the getNumPathDefinitions() method of pClassInfo to get the end condition for the loop. I think.

The question is if it isn't working why is nothing in the animations messed up?

The answer to that is that this entire function is never called (possibly because it doesn't work), at least that's what the Visual Studio "find all references" says and the read function immediately before it for the same class certainly doesn't. If you remove it (and the matching reference in the .h file) the DLL builds just fine. So I think the correct "code cleanup" would be to just remove the entire thing, not change one line that makes unused code that did nothing still do nothing.

And that is one reason the R2R updates take so long for me to do: I get sidetracked from time to time. And now I'm wondering how many of the many other similar functions that were added are not actually used. The very next one in the file, CvAnimationCategoryInfo::copyNonDefaults, is also apparently never used either...

You are right with the loop referencing the wrong vctPathDefinition.
I would not rely on "find all references" for this kind of virtual function that is often used in templates (which sometimes confuse Intellisense). As Koshling says, the copyNonDefaults method is used when an XML file is used in modular context. Then it fills in the other values.
I don't think this XML file would ever be used in modular entries that don't have the actual path defined so this bug won't ever have an effect (though it should still be fixed).
While you can remove copyNonDefault implementations and still build the DLL, that does not mean that it works fine as there is a base implementation in CvInfoBase for it.
C2C currently does not have the XML files belonging to this in the core so probably not in any module either but that might change at some point so I don't think we should remove the modular code.