[ZH] Fix iterator decrement before container begin in CountermeasuresBehavior::calculateCountermeasureToDivertTo

[Stubbjax]

Fixed an iterator underflow crash in the Countermeasures Behavior that could occur if a CountermeasuresVec object could not be found within the loop.

[Mauller]

i think to clearup this function it would be better to rewrite it to use rbegin and rend and use the reverse iterator from the container.

[xezon]

This does not crash in the retail (VC6) game, or does it?

[Mauller]

This code overall doesn't look like it has ever worked properly.

[Mauller]

I wrote and deleted this earlier since i wanted another look at it, but the following, in theory, should work to replace the original code

ObjectID CountermeasuresBehavior::calculateCountermeasureToDivertTo( const Object& victim )
{
	const CountermeasuresBehaviorModuleData *data = getCountermeasuresBehaviorModuleData();

	Real closestFlareDistance = 1e15f;
	Object *closestFlare = NULL;
	
	int volleySize = data->m_volleySize;
	int countedFlares = 0;

	//Flares are pushed to the front of the list, but we only want to acquire the "newest" of the flares, therefore
	//Start at the end of the list and go towards the beginning.
	CountermeasuresVec::reverse_iterator it = m_counterMeasures.rbegin();
	//stop iterating after we've reached size of a single volley.
	while( it != m_counterMeasures.rend() && countedFlares < volleySize )
	{
		Object *obj = TheGameLogic->findObjectByID( *it++ );
		if( obj )
		{
			Real weaponToFlareDist = ThePartitionManager->getDistanceSquared( obj, getObject(), FROM_CENTER_2D );
			if( weaponToFlareDist < closestFlareDistance )
			{
				closestFlareDistance = weaponToFlareDist;
				closestFlare = obj;
			}
			countedFlares++;
		}
	}

	if( closestFlare )
	{
		return closestFlare->getID();
	}
	return INVALID_ID;
}

[Caball009]

I wrote and deleted this earlier since i wanted another look at it, but the following, in theory, should work to replace the original code

Code looks good except that I imagine the original code worked even if data->m_volleySize == 0.

[Mauller]

Just checking this again, i realised that the original code does not actually do what it was meant to.

Essentially it will get stuck on the first iterator from the countermeasures list that returns a valid object then keep decrementing iteratorMax till it runs down.

So essentailly it never iterates over the possible available flares to see which is the closest to the approaching weapon. And just returns the first flare it comes across from the list.

So we might have to go with @Stubbjax 's fix for retail compat but add my fix as non retail compat as it should correct the behaviour and make flares work properly.

[Mauller]

To make this retail compatible and work in a similar fashion to the retail version, we could just return on the first object that gets returned, it would need a bit of testing but it only needs a small tweak to the code i posted

ObjectID CountermeasuresBehavior::calculateCountermeasureToDivertTo( const Object& victim )
{
	const CountermeasuresBehaviorModuleData *data = getCountermeasuresBehaviorModuleData();

	Real closestFlareDistance = 1e15f;
	Object *closestFlare = NULL;
	
#if RETAIL_COMPATIBLE_CRC
	int volleySize = MAX( data->m_volleySize, 1 );
#else
	int volleySize = data->m_volleySize;
#endif
	int countedFlares = 0;

	//Flares are pushed to the front of the list, but we only want to acquire the "newest" of the flares, therefore
	//Start at the end of the list and go towards the beginning.
	CountermeasuresVec::reverse_iterator it = m_counterMeasures.rbegin();
	//stop iterating after we've reached size of a single volley.
	while( it != m_counterMeasures.rend() && countedFlares < volleySize )
	{
		Object *obj = TheGameLogic->findObjectByID( *it++ );
		if( obj )
		{
			Real weaponToFlareDist = ThePartitionManager->getDistanceSquared( obj, getObject(), FROM_CENTER_2D );
			if( weaponToFlareDist < closestFlareDistance )
			{
				closestFlareDistance = weaponToFlareDist;
				closestFlare = obj;
			}
#if RETAIL_COMPATIBLE_CRC
			break;
#else
			countedFlares++;
#endif
		}
	}

	if( closestFlare )
	{
		return closestFlare->getID();
	}
	return INVALID_ID;
}

[Caball009]

To make this retail compatible and work in a similar fashion to the retail version, we could just return on the first object that gets returned, it would need a bit of testing but it only needs a small tweak to the code i posted

Again, what happened to Int iteratorMax = MAX( data->m_volleySize, 1 ); ? In your version the loop body will not execute if data->m_volleySize == 0 somehow.

[Mauller]

Again, what happened to Int iteratorMax = MAX( data->m_volleySize, 1 ); ? In your version the loop body will not execute if data->m_volleySize == 0 somehow.

Ah i get what you mean now, i tweaked it to add that as a retail compatible component.

With how the rest of the code reads along with the comments, it shouldn't do anything if there was no volley launched.

[Caball009]

Could make the macros a bit shorter:

	int volleySize = data->m_volleySize;

#if RETAIL_COMPATIBLE_CRC
	volleySize = MAX( data->m_volleySize, 1 );
#endif

#if RETAIL_COMPATIBLE_CRC
			break;
#endif
			countedFlares++;

Or does this result in a compiler warning? (unreachable code).

[Mauller]

Could make the macros a bit shorter:

	int volleySize = data->m_volleySize;

#if RETAIL_COMPATIBLE_CRC
	volleySize = MAX( data->m_volleySize, 1 );
#endif

#if RETAIL_COMPATIBLE_CRC
			break;
#endif
			countedFlares++;

Or does this result in a compiler warning? (unreachable code).

It would likely create a compiler warning, but what i did above is also in line with how we have treated other retail compatible fixes where code gets swapped in the same line.

The suggested change i made also needs testing still, would need to create a replay in retail first.

EDIT: this does not mismatch with the golden replays, but then i don't know if they used the countermeasures, might have to make a replay explicitly to test that.

[Mauller]

So i just did a quick test and the above is fully retail compatible in the retail compatible conditional.

The non retail compatible path is not compatible but it fully corrects the behaviour.

[xezon]

Ok can you open a pull for that or update this one?

[Mauller]

Ok can you open a pull for that or update this one?

i can have a look at doing that unless @Stubbjax wanted to update his commit?