4
\$\begingroup\$

I am trying to more or less perfect a movement script that activates when "triggered" by another object. Below I have the code for both the triggering object as well as the object being triggered.

I tried to provide summary data to make it obvious what I'm doing, but what can I improve here? And please don't tell me "everything". I am new to Unity3d, and am very much still in the process of learning. I need to know specifics on what I can do and work to improve my skill. Also, I know the models aren't the best thing ever made, but I'm also new to Blender!

Button / trigger object as well as its inspector

Button or Trigger Object Trigger Object In Inspector

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

public class PropActivateMovement : MonoBehaviour {

    #region Public Declarations
    private enum ActivationStyle 
    {
        OnTriggerEnter = 0,
        OnActionButton = 1,
        OnTriggerOccupy = 2
    }
    public Animator animActivation = null;
    #endregion

    #region Public Declarations
    public string AnimationActivationName = "PushTrigger";
    /// <summary>
    /// The audio source when this object is activated.
    /// </summary>
    public AudioSource AudioActivated = null;
    /// <summary>
    /// The object to move.
    /// </summary>
    public PropMovement ObjectToMove = null;
    /// <summary>
    /// The direction to move this object.
    /// A value greater than or equal to 1 moves in a positive direction.
    /// A value less than or equal to -1 moves in a negative direction.
    /// A value equal to 0 alternates direction.
    /// </summary>
    public int ObjectMovementDirection = 0;
    /// <summary>
    /// The style of activation for this object.
    /// A value of 0 activates the object OnTriggerEnter.
    /// A value of 1 activates the object only once pressing the action button while inside of the triggering location.
    /// A value of 2 activates the object OnTriggerEnter and deactivates the object OnTriggerExit.
    /// </summary>
    public int StyleOfActivation = 0;
    public bool IsActivated = false;
    #endregion

    private void Start() {
        AudioActivated = GetComponent<AudioSource> ();
    }

    #region Private Methods
    private void OnTriggerEnter () {
        Debug.Log ("Trigger area entered");
        if (StyleOfActivation == (int)ActivationStyle.OnTriggerEnter)
        {
            ActivateMovement();
        }
        else if (StyleOfActivation == (int)ActivationStyle.OnTriggerOccupy)
        {
            ActivateMovement();
        }
        animActivation = GetComponent<Animator> ();
    }

    private void OnTriggerExit () {
        Debug.Log ("Trigger area exited");
        if (StyleOfActivation == (int)ActivationStyle.OnTriggerOccupy)
        {
            ObjectToMove.SwitchMovement();
        }
    }

    private void OnTriggerStay () {
        if (StyleOfActivation == (int)ActivationStyle.OnActionButton)
        {
            if (Input.GetButtonDown("Action"))
            {
                Debug.Log("Action button pressed.");
                ActivateMovement();
            }
        }
    }
    public void ActivateMovement () {
        Debug.Log ("Activating prop movement.");
        if (AudioActivated != null)
        {
            AudioActivated.Play ();
        }
        if (animActivation != null)
        {
            int currentState = Animator.StringToHash ("PushTrigger");
            animActivation.Play(currentState);
        }

        if (ObjectMovementDirection == 0)
        {
            ObjectToMove.SwitchMovement();
        }
        else if (ObjectMovementDirection > 0)
        {
            ObjectToMove.MovePositive();
        }
        else if (ObjectMovementDirection < 0)
        {
            ObjectToMove.MoveNegative();
        }
        IsActivated = !IsActivated;
    }
    #endregion
}

Door to be triggered as well as its inspector

Door to be Triggered enter image description here

using UnityEngine;
using System.Collections;

public class PropMovement : MonoBehaviour {

    #region Private Declarations
    private Transform objTransform = null;
    private float startTime = 0.0f;
    private float DistanctToTravel = 0.0f;
    private Vector3 posObjectStart;
    private Vector3 posObjectCurrent;
    private bool isMovingPositive = false;
    #endregion

    #region Public Declarations
    /// <summary>
    /// The audio during movement.
    /// </summary>
    public AudioSource AudioDuringMovement = null;
    /// <summary>
    /// The object is currently moving.
    /// </summary>
    public bool isMoving = false;
    /// <summary>
    /// The position in which the object should stop movement.
    /// </summary>
    public Vector3 posObjectEnd;
    /// <summary>
    /// Smoothing factor of the doors movement.
    /// </summary>
    public float objectSmoothing = 1.0f;
    #endregion

    private void Start () {
        objTransform = transform;
        posObjectStart = transform.position;
    }

    #region Private Methods
    private void FixedUpdate() {
        if (posObjectEnd.x == 0.0f)
        {
            posObjectEnd.x = posObjectStart.x;
        }
        if (posObjectEnd.y == 0.0f)
        {
            posObjectEnd.y = posObjectStart.y;      
        }
        if (posObjectEnd.z == 0.0f)
        {
            posObjectEnd.z = posObjectStart.z;
        }
    }

    private void Update() {
        if (objTransform != null)
        {
            if (isMoving)
            {
                posObjectCurrent = objTransform.position;
                if (isMovingPositive)
                {
                    if (transform.position != posObjectEnd)
                    {
                        StartCoroutine(MoveObject(posObjectCurrent, posObjectEnd));
                    }
                    else
                    {
                        isMoving = false;
                    }
                }
                if (!isMovingPositive)
                {
                    if (transform.position != posObjectStart)
                    {
                        StartCoroutine(MoveObject(posObjectCurrent, posObjectStart));
                    }
                    else
                    {
                        isMoving = false;
                    }
                }
            }
            else
            {
                AudioDuringMovement.Stop();
            }
        }
    }

    private IEnumerator MoveObject(Vector3 moveFrom, Vector3 moveTo){
        float distCovered = (Time.time - startTime) * objectSmoothing;
        float smoothing = distCovered / DistanctToTravel;
        transform.position = Vector3.Lerp(moveFrom, moveTo, smoothing);
        yield return null;
    }

    private IEnumerator PlayMovementAudio() {
        AudioDuringMovement.Play();
        yield return null;
    }
    #endregion

    #region Public Methods
    /// <summary>
    /// Moves object in a positive direction.
    /// </summary>
    public void MovePositive(){
        StartCoroutine (PlayMovementAudio ());
        startTime = Time.time;
        isMovingPositive = true;
        isMoving = true;
        DistanctToTravel = Vector3.Distance (posObjectEnd, posObjectStart);
    }
    /// <summary>
    /// Moves object in a negative direction.
    /// </summary>
    public void MoveNegative(){
        StartCoroutine (PlayMovementAudio ());
        startTime = Time.time;
        isMovingPositive = false;
        isMoving = true;
        DistanctToTravel = Vector3.Distance (posObjectStart, posObjectEnd);
    }
    /// <summary>
    /// Moves object in a opposite direction of its previous movement.
    /// </summary>
    public void SwitchMovement(){
        if (isMovingPositive)
        {
            MovePositive();
        }
        else
        {
            MoveNegative();
        }
        isMovingPositive = !isMovingPositive;
    }
    #endregion
}
\$\endgroup\$
5
\$\begingroup\$

I recommend you don't use public fields in most circumstances. See this link for more information.

I am aware that Unity's inspector will not serialize properties, but there's a nifty way around it, you can use this pattern:

[SerializeField]
private float backingFloat;

public float PropertyFloat
{
    get
    {
        return backingFloat;
    }
    set
    {
        backingFloat = value;
    }
}

The 'SerializeField' attribute will expose the private field in the inspector. This way you can still set (and change at runtime) the value of a field using Unity's inspector, and also retain the joys of properties.

The only caveat you must remember is that any setter logic you write will not be applied to changes you make in the inspector, because the value will be written straight to the backing field. It's a minor pain for debugging and development, but will not affect a published game.

If you find the pattern a bit much, I recommend creating a snippet using your favourite IDE (it's possible in both MonoDevelop and Visual Studio, at the very least) to automatically create such properties for you.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Ahh! I wanted to do that per the standard class construction, but couldn't figure out how to make the property visible! Thanks! \$\endgroup\$ – Volearix Nov 24 '14 at 16:12
2
\$\begingroup\$
  1. Standard C# naming convention for public fields is PascalCase.

  2. You are mixing bracing styles. Sometime you put the starting { on a new line and sometimes you put it at the end of the previous line. Be consistent.

  3. In PropActivateMovement you have a bunch of public fields (which has already been mentioned as something you should avoid). Some of these fields are initialized with null yet you have code which relies on them not being null. If you have properties which have to have been initialized properly for your operations on the class to make sense then you should have a constructor which requires these as parameter possibly garnished with a ArgumentNullException in case someone passes in null. Some something along these lines:

    public class SomeClass 
    {
        public SomeClass(Foo foo, Bar bar)
        {
            if (foo == null)
                throw new ArgumentNullException("foo");
            if (bar == null)
                throw new ArgumentNullException("bar");
    
            ...
        }
    }
    
  4. MovePositive and MoveNegative contain almost the same code. This should be extracted into a common method instead.

  5. In the Update method there is also a fair amount of code duplication in this part:

    if (isMovingPositive)
    {
        if (transform.position != posObjectEnd)
        {
            StartCoroutine(MoveObject(posObjectCurrent, posObjectEnd));
        }
        else
        {
            isMoving = false;
        }
    }
    if (!isMovingPositive)
    {
        if (transform.position != posObjectStart)
        {
            StartCoroutine(MoveObject(posObjectCurrent, posObjectStart));
        }
        else
        {
            isMoving = false;
        }
    }
    

    This can be shortened to:

    var targetPosition = isMovingPositive ? posObjectEnd : posObjectStart;
    
    if (transform.position != targetPosition)
    {
        StartCoroutine(MoveObject(posObjectCurrent, targetPosition));
    }
    else
    {
        isMoving = false;
    }
    
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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