This nested FOR loop feels dirty and wrong... help?

Let me try to explain what I’m doing in the code below, which just feels so unclean and wrong… I’m struggling to find a way to do this.

Imagine I have a vehicle that has two weapons of the same type, firing from different locations on the vehicle. They have a “Shot Delay” of one second, so every second, both weapons release one piece of ordnance/projectile at the same time. I have all this working, but I now want to add a flexible system that allows me to offset the fire time of each weapon based on how many duplicates of it there are. I’m calling this flag ‘Shot Alternate’. If the weapons in this scenario had this set to true, then the first weapon would fire, then 0.5 of a second later (Shot Delay / Num Of This Weapon), the second weapon would fire. So on and so forth.

I want this to be the case regardless of how many weapons of the same type are on the craft. So if there are three, they will fire sequentially every 0.33 seconds (approximately). You get the idea. Essentially if a Sub-Class of ABZGame_Weapon has this flag set to true, whenever mounted on a vehicle it will cycle the shots between each ‘Hardpoint’.

This is where I’m heading with it so far, which just feels incredibly long-winded. The hardpoints of the craft may change at runtime as craft collect powerups and replace weapons, so it doesn’t have to be run often, but it still feels long-winded.

My idea is the following.

1 - Store a float ‘TimeOffset’ in each weapon class. (The weapons are of the Actor class, like ShooterGame).
2 - Offset the float in each weapon in the GameObjectComponent, based on how many duplicates of it there are in the same firing group*.
3 - This needs to change and update / reset when the loadout changes.

*The Firing-Groups are arrays of weapons that fire at the same time. The player can select different banks of weapons to fire.

This code is obviously wrong anyway, and I haven’t got as far as applying the offsets… But it’s the general direction I’m heading in. Can somebody possibly suggest a better way to do this?



void UBZGame_GameObjectComponent::UpdateWeaponTimeOffsets()
{
	TArray<ABZGame_Weapon*> UniqueWeaponTypes;
	TArray<int32> NumberOfDuplicates;

	/* Search the Hardpoints for Weapon Types. Only add the Unique Weapon types that have ShotAlternate to TRUE. */
	for (int32 i = 0; i < HardPoints.Num(); i++)
	{
		if (HardPoints*.CurrentWeapon && (HardPoints*.CurrentWeapon->WeaponConfig.ShotAlternate == true))
		{
			UniqueWeaponTypes.AddUnique(HardPoints*.CurrentWeapon)
		}
	}

	/* Set Num Duplicates To Same Length as Unique Wep Types */
	NumberOfDuplicates.Init(UniqueWeaponTypes.Num());

	/* For each of the unique weapon types, count how many are in the array. */
	for (int32 i = 0; i < UniqueWeaponTypes.Num(); i++)
	{
		/* Loop through the array and count how many of each unqiue weapon type we have. */
		for (int32 n = 0; n < HardPoints.Num(); n++)
		{
			if (HardPoints[n.CurrentWeapon && (HardPoints[n].CurrentWeapon->WeaponConfig.ShotAlternate == true) && HardPoints[n].CurrentWeapon->IsA(UniqueWeaponTypes*))
			{
				NumberOfDuplicates* += 1;
			}
		}
	}

	// DO OFFSET STUFF HERE
}


If I understand your setup correctly then all you have to do is this:

Have a static CumulativeOffset float in your class that starts at 0. Each weapon with the flag sets its offset to MyBaseOffset + CumulativeOffset, then adds its BaseOffset to the CumulativeOffset.

Once a weapon fires it waits for CumulativeOffset seconds before firing again.

So say you have 3 weapons with 1s, 1s and 2s delay respectively.

The first weapon’s offset is 1 + 0 seconds, and the cumulative offset now is 1.

Second weapon has a 1 + 1 second offset, the cummulative offset is now 2.

Third gun has a 2 + 2 second offset and the cummulative offset is now 4.

So the first gun will fire at second 1, then next time in second 5 etc.

You can rerun this calculation on all guns when guns are added or removed to the chain or when delays change. As long as all guns are kept referenced in some array so you don’t have to look them up all the time it’s cheap.

Looking at things strictly from an algorithmic perspective, I don’t think you need that 2nd and 3rd iteration pass at all, if I’m understanding things correctly. You could just be a bit smarter when you are constructing your initial list.


	int32 foundIndex = -1;
	for (int32 i = 0; i < HardPoints.Num(); i++)
	{
		if (HardPoints*.CurrentWeapon && (HardPoints*.CurrentWeapon->WeaponConfig.ShotAlternate == true))
		{
			foundIndex = UniqueWeaponTypes.Find(HardPoints*.CurrentWeapon);
			if (foundIndex == -1) // Doesn't exist, add it to our list.
			{
				UniqueWeaponTypes.Add(HardPoints*.CurrentWeapon)
				NumberOfDuplicates.Add(0); // Add an entry to our duplicate tracker.
			}
			else
			{
				NumberOfDuplicates[foundIndex]++; // It's a duplicate, add to our counter for this type.
			}
				
		}
	}

Now you only have to iterate through all your weapons once, instead of (worse case) 3 times.

Thanks AdeptStrain, I have to confess I’ve been typing out like this and backspacing for a couple of days now… that code I posted is probably way less than optimized.

BTW I should add: The Static won’t necessarily work in my case since there are up to a few hundred of these in the world at once. And it’s multi-player, so I have to replicate whatever I do to the weapons too.

I think it might help to work backwards here: How do you want the firing logic to work? I say this because this might inform how you want to build the lists you’re talking about.

Personally I would probably do this:

Store a group of weapons that fire together (I think you said you’re doing this).
- Those weapons might fire in sequential or linked mode (think lasers on an X-Wing). Sequential you might cycle between 4 lasers with 1 second between shots, 2-way link you would fire 2 lasers with 2 seconds between, etc.
- Firing timing logic could be done at the group level (per weapon type), which makes this simpler I think. CurrentFiringIndex = which weapon to fire next. NextTimeToFire = when you can fire.
- When you shoot, check if CurrentTime >= NextTimeToFire. If so, fire however many lasers are linked (for simplicity say just 1), starting at CurrentFiringIndex. Advance the index, update NextTimeToFire, and you’re done. If you fire more than one at a time, you fire them all, and advance the index accordingly.

I’m not clear on if you allow different weapon types within a “group”. My solution above is based on weapons with the same DelayBetweenShots in a single group. I think if you group them at that level it’s pretty simple. Creating the group is probably just a Sort() of the list by weapon type, and each chain of duplicates becomes it’s own linked group.

Thanks Zak! You’re pretty much spot-on about the X-wing thing. The firing index system sounds like a good idea… what I’ve got right now is a very similar set-up to ShooterGame, so the weapon handles the firing all on it’s own - It’s just told to ‘start’ firing like so. All the weapons I currently have selected want to fire sequentially, so long as they’re the same weapon and have ShotAlternate set to true.

All I’m doing right now to fire the weapons and “group” them is this (I thought I grouped them into an array but apparently I didn’t). At the moment I don’t set any different weapon types in the same group (though, there’s no reason somebody else couldn’t). Would the system you have there be okay via network gameplay do you think, I wouldn’t have to synchronize time or anything? If so I probably don’t have to replicate anything extra using that approach either.

Here’s how the firing / grouping works currently. It’s more psuedo-grouping really, since they never get grouped into arrays of their own (though, it might be good to do so). Also, no idea why I’m doing two ‘Weapon’ checks in StartWeaponFire()… that can be removed.



// START WEAPON FIRE
void UBZGame_GameObjectComponent::StartWeaponFire()
{
	if (!bWantsToFire)
	{
		bWantsToFire = true;

		for (int32 i = 0; i < HardPoints.Num(); i++)
		{
			ABZGame_Weapon* Weapon = Cast<ABZGame_Weapon>(HardPoints*.CurrentWeapon);
			if (Weapon && HardPoints*.FiringGroup == ActiveWeaponGroup)
			{
				if (Weapon)
				{
					Weapon->StartFire();
				}
			}
		}
	}
}

// UPDATE THE AMOUNT OF GROUPED WEAPONS
void UBZGame_GameObjectComponent::UpdateTotalWeaponGroups()
{
	TotalWeaponGroups;
	TArray<uint32> FiringGroupArray;

	// For every hard-point in the inventory, add the firing group to an array of ints. 
	for (int32 i = 0; i < HardPoints.Num(); i++)
	{
		ABZGame_Weapon* Weapon = Cast<ABZGame_Weapon>(HardPoints*.CurrentWeapon);
		// For every firing group that actually has a current weapon 
		if (Weapon)
		{
			if (!FiringGroupArray.Contains(HardPoints*.FiringGroup))
			{
				FiringGroupArray.Add(HardPoints*.FiringGroup);
			}
		}
	}

	TotalWeaponGroups = FMath::Max<uint32>(FiringGroupArray);
}

// SELECT NEXT WEAPON GROUP
void UBZGame_GameObjectComponent::SelectNextWeaponGroup()
{
	StopWeaponFire();
	ActiveWeaponGroup += 1;

	if (ActiveWeaponGroup <= 0)
	{
		ActiveWeaponGroup = TotalWeaponGroups;
	}
	else if (ActiveWeaponGroup > TotalWeaponGroups)
	{
		ActiveWeaponGroup = 1;
	}
}


Got it working! Even managed to keep the code mostly as part of the weapon class to, and it requires only one additional replicated variable that should only replicate when a new weapon of the same class is added, or when they are initially spawned in.



void ABZGame_Weapon::OnEnterInventory(APawn* NewOwner, UBZGame_GameObjectComponent* NewGameObject)
{
	SetOwningPawn(NewOwner);
	SetOwningGameObject(NewGameObject);

	// TODO: Check for mouse button held down

	/* If this is an alternating-shot weapon. */
	if (WeaponConfig.ShotAlternate)
	{
		int32 SequenceNum = 0;
		TArray<ABZGame_Weapon*> AllCopies;

		/* For each hard-point, try to find any duplicates of this item, and count them. */
		for (int32 i = 0; i < NewGameObject->HardPoints.Num(); i++)
		{
			if (NewGameObject->HardPoints*.CurrentWeapon && WeaponName == NewGameObject->HardPoints*.CurrentWeapon->GetWeaponName())
			{
				/* If we find one, increase the sequence number, and add the duplicate to the array of copies */
				SequenceNum++;
				AllCopies.Add(NewGameObject->HardPoints*.CurrentWeapon);
			}
		}

		/* Set this weapons sequence number to be the current amount of copies */
		SequentialIndex = SequenceNum;

		/* For every individual copy of this weapon, set the number of duplicates that exist and update it's initial delay */
		for (ABZGame_Weapon* AllCopyItr : AllCopies)
		{
			AllCopyItr->SetNumDuplicates(SequenceNum);
		}
	}
}

void ABZGame_Weapon::SetNumDuplicates(int32 InNumDuplicates)
{
	NumDuplicates = InNumDuplicates;

	if (NumDuplicates > 0)
	{
		SequentialDelay = (WeaponConfig.ShotDelay / NumDuplicates) * (SequentialIndex - 1);
	}
}


Then in ‘StartFire’, I simply delay the call to ‘DetermineWeaponState’ by this weapons Sequence delay value. Easy.



void ABZGame_Weapon::StartFire()
{
	if (Role < ROLE_Authority)
	{
		ServerStartFire();
	}
	if (!bWantsToFire)
	{
		bWantsToFire = true;

		if (WeaponConfig.ShotAlternate && SequentialDelay > 0.f)
		{
			GetWorldTimerManager().SetTimer(SequentialTimer, this, &ABZGame_Weapon::DetermineWeaponState, SequentialDelay, false);
		}
		else
		{
			DetermineWeaponState();
		}
	}
}

void ABZGame_Weapon::StopFire()
{
	if (Role < ROLE_Authority)
	{
		ServerStopFire();
	}

	if (bWantsToFire)
	{
		bWantsToFire = false;
		DetermineWeaponState();
	}

	if (GetWorldTimerManager().TimerExists(SequentialTimer))
	{
		GetWorldTimerManager().ClearTimer(SequentialTimer);
	}
}