5
\$\begingroup\$

Here is my missile launcher script, and it is in my opinion borderline acceptable code except for how resource expensive it is. I was wondering if anyone could help me write it better so I could learn a thing or two.

Weapon code:

    [SerializeField]
    private float lockOnTime;
    [SerializeField]
    private GameObject lockOnPrefab;
    [SerializeField]
    private Transform lockOnParent;
    private Dictionary<GameObject, GameObject> targetDict = new Dictionary<GameObject, GameObject>();
    public bool isLockedOn = false;
    public float currentTime;

    private LockOn previousLockOn;
    private int lockOnLayerMask;

    protected override void Start ()
    {
        base.Start();
        lockOnLayerMask = LayerMask.GetMask("LockOnLayer");
        currentTime = lockOnTime;
    }

    protected override void OnDisable()
    {
        base.OnDisable();
        foreach (GameObject go in targetDict.Values)
        {
            Destroy(go);
        }
        targetDict.Clear();     
    }

    protected override void Update()
    {
        base.Update();

        ShowPotentialTargets();
    }

    public override bool Shoot()
    {
        if (isLockedOn && base.Shoot())
        {
            foreach (Transform t in prefabSpawnPoints)
            {
                //GameObject newMissle = (GameObject)Instantiate(instantiatePrefab, t.position, t.rotation);
                //newMissle.GetComponent<Missle>().SetTarget(lockOnTransform);
                currentAmmo -= 1;
            }
            return true;
        }
        return false;
    }

    public override void Aim(bool isPressed)
    {
        base.Aim(isPressed);

        RaycastHit hit;
        bool rayHitSomething = Physics.Raycast(transform.position, transform.forward, out hit, range, lockOnLayerMask);
        if (rayHitSomething && targetDict.ContainsKey(hit.transform.gameObject))
        {
            if (isPressed)
            {
                currentTime -= Time.deltaTime;
                if (currentTime <= 0)
                {
                    isLockedOn = true;
                    currentTime = 0;
                }
            }
            else
            {
                currentTime += Time.deltaTime;
                if (currentTime >= lockOnTime)
                {
                    currentTime = lockOnTime;
                }
                isLockedOn = false;
            }

            LockOn currentLockOn = targetDict[hit.transform.gameObject].GetComponent<LockOn>();
            currentLockOn.isSelected = true;
            currentLockOn.percent = 1 - currentTime / lockOnTime;
            if (hit.transform.tag == "EnemyLockOnCollider")
            {
                currentLockOn.SetUpColor(Color.red);
            }
            else if (hit.transform.tag == "AllyLockOnCollider")
            {
                currentLockOn.SetUpColor(Color.green);
            }
            if (previousLockOn != currentLockOn)
            {
                if (previousLockOn != null)
                {
                    previousLockOn.isSelected = false;
                }
                previousLockOn = currentLockOn;
            }
        }
        else if (previousLockOn != null)
        {
            previousLockOn.isSelected = false;
        }
    }

    public void ShowPotentialTargets()
    {
        foreach (GameObject target in GameObject.FindGameObjectsWithTag("EnemyLockOnCollider"))
        {
            Vector3 screenPoint = Camera.main.WorldToViewportPoint(target.transform.position);
            bool onScreen = screenPoint.z > 0 && screenPoint.x > 0 && screenPoint.x < 1 && screenPoint.y > 0 && screenPoint.y < 1;
            if (onScreen && Vector3.Distance(transform.position, target.transform.position) < range)
            {
                if (!targetDict.ContainsKey(target))
                {
                    GameObject targetImage = (GameObject)Instantiate(lockOnPrefab, lockOnParent);
                    LockOn lockOnScript = targetImage.GetComponent<LockOn>();
                    lockOnScript.SetUpTarget(target.transform);
                    targetDict.Add(target, targetImage);
                }
            }
            else
            {
                if (targetDict.ContainsKey(target))
                {
                    Destroy(targetDict[target]);
                    targetDict.Remove(target);
                }
            }
        }
    }

Lock On Code (this goes on the targetet game object):

private Transform target;
public bool isSelected;
public float percent;

[Header("Scale")]
[SerializeField]
private Vector3 startScale;
[SerializeField]
private Vector3 finalScale;

[Header("Color")]
[SerializeField]
private Color defaultColor;
[SerializeField]
private Color startColor;
private Color finalColor;

private RectTransform rect;
private Image image;

void Start()
{
    rect = GetComponent<RectTransform>();

    image = GetComponent<Image>();

    rect.localScale = startScale;       
}

void Update()
{
    transform.position = Camera.main.WorldToScreenPoint(target.position);

    if (isSelected)
    {
        image.color = Color.Lerp(startColor, finalColor, percent);

        rect.localScale = Vector3.Lerp(startScale, finalScale, percent);
    }
    else
    {
        image.color = defaultColor;
    }
}

public void SetUpTarget(Transform target)
{
    this.target = target;
}

public void SetUpColor(Color finalColor)
{
    this.finalColor = finalColor;
}

This code does what i want. It is from a class missle launcher that derives from weapon. When i see the targets on screen they turn white, if a ray hits them that means they are in front of the plane and it starts locking on when i aim with right mouse. When i am fully locked on the target turns red and i am able to shoot at it. My problem is that there is a search for all gameobjects every frame and so many if's. It is really reesource heavy and i dont know how to make it run faster. For example: Caching all targets would be beter for the performance but i dont know when they go off screen and so on... If anyone has time I would really appreciate the help!

\$\endgroup\$
2
  • 2
    \$\begingroup\$ "This code does what i want" Which is what? There is no problem description in your question. When is this called? Why is this called? \$\endgroup\$
    – Mast
    Nov 4, 2017 at 8:45
  • 2
    \$\begingroup\$ Ok edited it my bad. \$\endgroup\$
    – Nik Vozelj
    Nov 4, 2017 at 11:16

1 Answer 1

1
\$\begingroup\$

Performance

While it is difficult, or next to impossible, to judge the performance from the code alone, there are some things that are likely to be causing the performance issues you are experiencing. I will assume that you actually do have performance issues (e. g. low framerates or frame stuttering), or have profiled it and found out that you are very close to the resource limit, thus likely to run into performance problems on slightly weaker hardware. If none of that applies, I would not recommend working on performance improvements until they arise (although it is of course good to have performance in mind from the start when making decisions that are likely to have a meaningful impact on performance).

Instantiate, Destroy and FindGameObjectsWithTag are the operations which are most likely quite slow considering how often they are done. I recommend having a look at the object pool pattern. In the simplest case, you can just add the instantiated objects to some collection (e. g. a Set) and iterate over that instead of doing FindGameObjectsWithTag. Or, since you already seem to be using targetDict in a similar way, can you simply iterate over that?

Another possibility worth considering is to use a coroutine and run this resource-heavy algorithm only e. g. every 10 frames. If the lock-on mechanism is allowed to have some slight delay of a few frames without the player noticing, you could potentially increase your framerate quite a bit. Note though that, although coroutines are executed asynchronously, they generally do not run in parallel, so depending on how resource-intensive the algorithm actually is you might still experience some frame stuttering, albeit with a higher framerate overall, so it might simply be a bit less smooth than ideal.

You might get a small performance boost from using CompareTag() instead of comparing the tags manually (see https://answers.unity.com/questions/200820/is-comparetag-better-than-gameobjecttag-performanc.html). Depending on how often this is called, you might not notice a difference performance-wise, but the downside is basically zero, because it is still very readable.

Naming

  1. lockOnTime :
    Based on the context, it seems like lockOnDuration would be more accurate. While currentTime is a point in time (e. g. on the clock), the lockOnDuration is an amount of time without a specific start and end time.
  2. Dictionary<GameObject, GameObject> targetDict :
    This name, combined with the type GameObject for both keys and values, does not tell me what I find in the dictionary. I would have to look up how it is created or how values are added in order to understand what are the keys and what are the values.
    Suggestion: lockOnMarkersByLockOnTarget tells me that the keys are the targets and the values are the markers for each target, and that I can query a marker via the respective target. Note that I am making an assumption, since the name itself should be telling me the full intent, so if my suggested name is not accurate, I recommend applying the same naming pattern but with the correct meaning.
  3. percent :
    Percentages range from 0 to 100, but in this case we have a value between 0.0 and 1.0, so percent is inaccurate and misleading.
    Suggestions: fraction, progress, or progressNormalized

Structure

  1. You can reduce the number of ifs, and increase readability in general, by extracting some methods.

For example this ...

if (hit.transform.tag == "EnemyLockOnCollider")
{
    currentLockOn.SetUpColor(Color.red);
}
else if (hit.transform.tag == "AllyLockOnCollider")
{
    currentLockOn.SetUpColor(Color.green);
}

... can be extracted into a method DetermineColor ...

private Color DetermineColor(string tag)
{
    switch (tag)
    {
        case "EnemyLockOnCollider":
            return Color.red;
        case "AllyLockOnCollider":
            return Color.green;
        default:
            throw new ArgumentException($"Unexpected tag '{tag}'.");
    }
}

... and called like this:

var color = DetermineColor(hit.transform.tag);
currentLockOn.SetUpColor(color);

See also:

  1. This ...
if (...)
{
    if (...)
    {
        ...
    }
}
else
{
    if (...)
    {
        ...
    }
}

... can be simplified by merging the ifs respectively if and else, because the inner ifs are the only contents of the outer blocks:

if (... && ...)
{
  ...
}
else if (...)
{
  ...
}

Inheritance

It is hard to guess what base.Start(), base.Update(), base.Aim(isPressed), etc. does. I would have to navigate into that file (which happens to not be available here), and in some cases the base class might not even be available as source code (only as a DLL, for example). For these and more reasons, inheritance should generally be avoided, with few exceptions. Instead, the common parts between this missile launcher and other Weapons could be in an object that is contained within the missile launcher class, e. g. in a variable, and then can be delegated to (i. e. composition over inheritance). As a made-up example, you could have something like magazine.Reload(); instead of base.Start();, even if the latter might internally be doing the same thing, but less explicitly. Shared behavior between multiple classes is not sufficient reason for using inheritance.

Logic

if (isPressed)
{
    currentTime -= Time.deltaTime;
    if (currentTime <= 0)
    {
        isLockedOn = true;
        currentTime = 0;
    }
}
else
{
    currentTime += Time.deltaTime;
    if (currentTime >= lockOnTime)
    {
        currentTime = lockOnTime;
    }
    isLockedOn = false;
}
// ...
currentLockOn.percent = 1 - currentTime / lockOnTime;

It seems like we count down from the lockOnTime (i. e. lockOnDuration) to 0. Because of that, we then later have to do 1 - ..., i. e. invert the calculated progress, to get an actual progress from 0 to 1. That can be very confusing when reading the code. My first assumption when reading percent = 1 - ... was that we actually end up with percentages from 100 to 0, and then also interpolate the colors in reversed order, which was a misinterpretation. Because of that, I would recommend counting upwards, or at least indicating through explicit naming why the currentTime is actually reversed, e. g. naming it remainingLockOnDuration or remainingTimeUntilLockOn.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.