Am I Misusing Abstract Classes or Delegates?

Unity Engine
1

Hey all. I finally found a way to make my missile launcher work the way I want it to, but I want to know if my solution is going to cause trouble ahead, or if it’s just poor coding to begin with.

First, some background info (You can probably skip over most of the code. It’s just there because people usually ask to see it):

I have a BombScript class from which my special bomb types (power-ups) are derived from. Usually, these are spawned by players as they are the primary mode of attack.

There is an AbstractTrapScript which I’d like to use for any “trip-able” trap I decide to implement. It looks like this:

AbstractTrapScript

using UnityEngine;
using System.Collections;

public abstract class AbstractTrapScript : MonoBehaviour
{
    public delegate void ActivateCallback();

    public abstract void Activate (ActivateCallback callBack = null);
}

… Pretty simple. The idea is that I’d like any switches that have traps attached to be able to call the Activate function to make the trap do whatever it does. Currently, the callback is used for telling the switch when to return to the “ready” state. For example, I have a Sawblade trap and a Laser trip-wire-like switch. The sawblades can be motionless, or they can move between any number of nodes. They can move on their own, or they can move when a Laser switch is tripped. In the inspector, I set the laser’s public AbstractTrapScript to whichever saw I want it to be associated with. Here’s a quick demo of it working:

Notice that the laser switch doesn’t work until the saw reaches its destination. That’s because the switch changes to an inactive state until the LaserScript’s “SetReady” function is called, which happens in the SawbladeScript:

TripLaserScript

using UnityEngine;
using System.Collections;

// direction is set in the inspector.  Yes, it is supposed to be outside of the class:
public enum CardinalDirection {RIGHT, LEFT, UP, DOWN};

public class TripLaserScript : MonoBehaviour
{
    private LineRenderer line;
    public AbstractTrapScript trap;                                    // this is the (node-movement-based) trap affected by the switch
    public LayerMask laserLayerMask;
    public int targetNode = 0;                                        // the node that this trip wire moves the node-based object to
    private Color _activeColor = new Color32(255, 0, 0, 170);        // the colour the laser should be when ready to be tripped again
    private Color _inactiveColor = new Color32(0, 255, 0, 170);        // the colour the laser should be when not ready to be tripped
    private bool ready = true;                                        // the laser is ready to be tripped again.
    private bool fire = true;                                        // the laser will fire while true
    public CardinalDirection facing = CardinalDirection.RIGHT;
    private Vector2 aimDirection;
    private float originOffset = 0.17f; // was (and should probably be 0.17f)
    private Vector2 laserOrigin;
    private Animator anim;


    void Awake()
    {
        line = GetComponent<LineRenderer>();
        anim = GetComponent<Animator>();
    }

    // Use this for initialization
    void Start ()
    {
        SetAimDirection();

        if (trap)
            StartCoroutine(Fire());
        else
            Debug.LogError(this.name +  ": 'AbstractTrapScript' not assigned assigned!");
    }

    public void SetReady()
    {
        ready = true;
        line.SetColors(_activeColor, _activeColor);
        anim.SetBool("on", ready);
    }

    private IEnumerator Fire()
    {
        RaycastHit2D hit;
       
        while (fire)
        {
            // was this for a  long time.  commented out Feb-20-2016 to try to make up-facing lasers work
            //Ray2D ray = new Ray2D(new Vector2(transform.position.x + laserOrigin.x, transform.position.y + laserOrigin.y), aimDirection);

            Ray2D ray = new Ray2D(new Vector2(transform.position.x, transform.position.y), aimDirection);
           
            //line.SetPosition(0, ray.origin);
            hit = Physics2D.Raycast(ray.origin, ray.direction, 100, laserLayerMask);
           
            if (hit)
            {
                line.SetPosition(1, transform.InverseTransformPoint(hit.point));
                line.enabled = true;        // this is probably unnecessary !!!


                if (ready && !hit.collider.gameObject.GetComponent<GhostBombScript>() &&
                    (hit.collider.gameObject.GetComponent<PlayerController>() || hit.collider.gameObject.GetComponent<BombScript>()))
                {
                    line.SetColors(_inactiveColor, _inactiveColor);
                    trap.Activate(SetReady);
                    ready = false;
                    anim.SetBool("on", ready);
                }
            }
            else
                line.SetPosition(1, aimDirection * 100);

            yield return null;
        }
    }

    // sets the aimDirection Vector2 based on the facing enum
    private void SetAimDirection()
    {
        switch (facing)
        {
        case CardinalDirection.LEFT:
            aimDirection = -Vector2.right;
            laserOrigin = new Vector2(originOffset, 0);
            line.SetPosition(0, laserOrigin);
            break;
        case CardinalDirection.UP:
            aimDirection = Vector2.up;
            laserOrigin = new Vector2(originOffset, 0);
            line.SetPosition(0, laserOrigin);
            break;
        case CardinalDirection.DOWN:
            aimDirection = -Vector2.up;
            laserOrigin = new Vector2(0, -originOffset);
            line.SetPosition(0, laserOrigin);
            break;
        default:
            aimDirection = Vector2.right;
            laserOrigin = new Vector2(originOffset, 0);
            line.SetPosition(0, laserOrigin);
            break;
        }
    }
}

SawbladeScript

using UnityEngine;
using System.Collections;

public class SawbladeScript : AbstractTrapScript
{
    private string hazardName = "sawblade";
    private AudioSource audioSource;

    public GameObject[] nodes;
    public int nextNodeIndex = 0;
    public float speed = 5;
    public bool moveOnPlay = true;
    //public delegate void DestinationReached();        // a delegate function used to tell related switch that the object has reached its destination.
    //private DestinationReached reachedNode;
    private ActivateCallback reachedNode;
    private int currentNode = -1;
    private int targetNode = 0;

    void Awake()
    {
        audioSource = GetComponent<AudioSource>();
    }

    // Use this for initialization
    void Start ()
    {

        if (nodes.Length > 0 && moveOnPlay)
        {
            StartCoroutine(WaitForUnpause());
        }

    }

    // Update is called once per frame
    void Update ()
    {
        transform.RotateAround(transform.position, Vector3.forward, 600 * Time.deltaTime);
    }

    public void OnTriggerEnter2D(Collider2D col)
    {
        if (col.gameObject && col.gameObject.GetComponent<PlayerController>() &&
            col.gameObject.GetComponent<PlayerController>().IsAlive() && !col.gameObject.GetComponent<PlayerController>().IsInvincible())
        {
            col.gameObject.GetComponent<PlayerController>().Kill(hazardName);
            if (!audioSource.isPlaying)
                audioSource.Play();
        }
        else if (col.gameObject && col.gameObject.GetComponent<BombScript>() && !col.gameObject.GetComponent<BombScript>().GetExploded())
        {
            StartCoroutine(col.gameObject.GetComponent<BombScript>().InstantExplode());
            if (!audioSource.isPlaying)
                audioSource.Play();
        }
        else if (col.gameObject && col.gameObject.GetComponent<ItemCrateScript>() && !col.gameObject.GetComponent<ItemCrateScript>().IsExploded())
        {
            col.gameObject.GetComponent<ItemCrateScript>().TakeExplosionDamage();
            if (!audioSource.isPlaying)
                audioSource.Play ();
        }
    }

    // moves object from node-to-node in the list, returning to node 0 after the end is reached
    private IEnumerator MoveCycle()
    {
        while (Vector2.Distance(transform.position, nodes[nextNodeIndex].transform.position) > 0.1f)
        {

            transform.position = Vector2.MoveTowards(transform.position, nodes[nextNodeIndex].transform.position, speed * Time.deltaTime);

            yield return null;
        }

        // when the destination node is reached...
        transform.position = nodes[nextNodeIndex].transform.position;
        currentNode = nextNodeIndex;

        if (reachedNode != null)
            reachedNode();

        NextNodeInCycle();

        yield return StartCoroutine(MoveCycle ());
    }

    // moves object to the specified node and waits there
    private IEnumerator MoveTowardsNode(int next)
    {
        while (Vector2.Distance(transform.position, nodes[next].transform.position) > 0.1f)
        {

            transform.position = Vector2.MoveTowards(transform.position, nodes[next].transform.position, speed * Time.deltaTime);

            yield return null;
        }

        // when the destination node is reached...
        transform.position = nodes[next].transform.position;
        currentNode = next;

        if (reachedNode != null)
            reachedNode();
    }

    public void NextNodeInCycle()
    {
        int next = nextNodeIndex + 1;

        if (next > (nodes.Length - 1))
        {
            nextNodeIndex = 0;
        }
        else
            nextNodeIndex = next;
    }

    private IEnumerator WaitForUnpause()
    {
        while (PlayerSpawner.paused)
        {
            yield return null;
        }
           
        if (nodes.Length > 0)
            StartCoroutine(MoveCycle());
    }

    public void MoveToSpecifiedNode(int n)
    {
        nextNodeIndex = n;

        StartCoroutine(MoveTowardsNode(n));

    }

    // used if this object moves via switch
    public override void Activate(ActivateCallback f)
    {
        if (f != null)
            reachedNode = f;

        if (Vector2.Distance(transform.position, nodes[nextNodeIndex].transform.position) <= 0.1f)
            NextNodeInCycle ();

        MoveToSpecifiedNode(nextNodeIndex);
    }

    public int GetCurrentNode()
    {
        return currentNode;
    }
}

I know some of the method names in the Sawblade script are a little confusing. I need to clean it up.

So how am I so far? I don’t expect anyone to point out every little thing wrong, but if there’s anything glaring that I’m oblivious to, I’d be happy to hear the criticism.

The part I’m most concerned about, actually, is how I just coded the BombSpawnerScript. Until today, it was simply used to spawn bombs on timed (possibly random) intervals. But, I finally implemented a laser-controlled trap-- a missile-launcher. Trip the laser and a missile is fired. Of course, I can’t have it firing a constant stream of missiles. I thought it would be best if a missile isn’t fired until the last one is gone (exploded). Here’s the BombSpawnerScript

BombSpawnerScript

using UnityEngine;
using System.Collections;

// this script can be used to spawn bombs or crush blocks

public enum SpawnMode {OnRandomInterval, OnActivate};

public class BombSpawnerScript : AbstractTrapScript
{
    public GameObject bomb;
    private GameObject bombClone;

    public SpawnMode spawnMode = SpawnMode.OnRandomInterval;
   
    public float minSpawnTime = 1;
    public float maxSpawnTime = 5;

    public bool startFuse = false;
    public bool spawnOnPlay = false;
    public bool throwOnSpawn = false;
    public Vector2 throwDir;

    // Use this for initialization
    void Start ()
    {
        if (spawnOnPlay)
            SpawnBomb();

        if (spawnMode == SpawnMode.OnRandomInterval)
            StartCoroutine(StartSpawnTime(Random.Range(minSpawnTime, maxSpawnTime)));
        else if (spawnMode == SpawnMode.OnActivate)
        {
            throwOnSpawn = true;
            startFuse = true;
        }
    }
   
    // Starts the spawn process with a timer which starts if a bomb from this spawner is not already in the scene
    private IEnumerator StartSpawnTime(float spawnTime)
    {
        // Don't spawn a bomb if there is already one on the field or if the game is paused
        while (bombClone || PlayerSpawner.paused)
            yield return null;
       
        yield return new WaitForSeconds(spawnTime);
        SpawnBomb();
    }
   
    private void SpawnBomb()
    {
        bombClone = (GameObject) Instantiate(bomb, transform.position, transform.rotation);

        if (bombClone.GetComponentInChildren<CrushblockScript>())
        {
//            bombClone.GetComponent<SnapToGrid>().enabled = true;
//            bombClone.GetComponent<SetAnchorScript>().enabled = true;
//            bombClone.GetComponent<SnapToGrid>().enabled = false;
//            bombClone.GetComponent<SetAnchorScript>().enabled = false;
        }

        if (bombClone.GetComponent<BombScript>() && startFuse)
        {
            bombClone.GetComponent<BombScript>().Cook();

            if (throwOnSpawn)
            {
                if (throwDir == null)
                    throwDir = Vector2.zero;
               
                bombClone.GetComponent<BombScript> ().Throw (throwDir);
            }
            else
                bombClone.GetComponent<BombScript>().Drop();
        }

        if (spawnMode == SpawnMode.OnRandomInterval)
            StartCoroutine (StartSpawnTime(Random.Range(minSpawnTime, maxSpawnTime)));
    }

    public override void Activate(ActivateCallback callback)
    {
        // do a thing
        if (!bombClone)
        {
            SpawnBomb ();
            bombClone.GetComponent<BombScript> ().destroyCallback = callback;
        }
    }
}

It’s that last function I want to draw your attention to. I’m setting the bomb’s (missile’s) destroyCallback function to callback, which is the TripLaserScript’s SetReady function. Is this starting to look a little hairy? If so, it gets hairy-er…

The BombScript is not derived from AbstractTrapScript (actually, it’s derived from AbstractExplosiveScript, but that’s probably not important here). But I needed the bomb to be able to call this delegate function in its OnDestroy function. Here’s how I did that (most of the BombScript’s code is omitted):

BombScript

using UnityEngine;
using System.Collections;
using System.Collections.Generic;

public class BombScript : AbstractExplosive
{
    public AbstractTrapScript.ActivateCallback destroyCallback;

    void OnDestroy()
    {
        Debug.Log ("OnDestroy called");

        if (destroyCallback != null)
            destroyCallback ();
    }
}

See how I gave the BombScript an ActivateCallback delegate through the abstract AbstractTrapScript class? I’m surprised it even let me do that, because it looks insane to me. But it works!

Do you think I should leave it as-is unless/until it becomes a problem, or is it ok?

Thanks for reading. :slight_smile:

2

There’s nothing necessarily wrong with your use of abstract classes.

And it works, so why really change it unless necessary?

I will say though that your use of an abstract class is really what ‘interfaces’ are for.

Abstract classes usually are use for defining an interface (methods/properties that need to exist), as well as some basic implementation that all sub-classes will require.

Interfaces are for defining just the interface (methods/properties that need to exist).

For example your AbstractTrapScript could be instead:

public interface ITrap
{
    void Activate(System.Action callback);
}

(note - there are generic delegate types like Action that could be used instead of defining your Callback delegate above)

Then you can implement as follows:

using UnityEngine;
using System.Collections;

// this script can be used to spawn bombs or crush blocks

public enum SpawnMode {OnRandomInterval, OnActivate};

public class BombSpawnerScript : MonoBehaviour, ITrap
{
    //... all other stuff snipped for brevity ...//


    #region ITrap Interface

    public void Activate(System.Action callback)
    {
        // do a thing
        if (!bombClone)
        {
            SpawnBomb ();
            bombClone.GetComponent<BombScript>().destroyCallback = callback;
        }
    }

    #endregion
   
}

The nice thing about this is that you can actually implement multiple interfaces on one type.

Here’s an example of an abstract class for you:

Note how this supplies base functionality (not just method names).

2 Likes
3

Thanks, lordofduct. That’s just the sort of feedback I was looking for. I just made those changes and (with a few error fixes) it all seems to work just the same as before. Only now I have the satisfaction of knowing I did it properly (or at least, better).