5
\$\begingroup\$

All of this does just one thing. Move my player by finding the first finger that was used to touch the ship and drag it.

I have a few questions:

  • The first thing is I have an embedded class of MotionState. At first glance it makes a lot of sense to start with that. I latter ended up not using it. How do I go about refactoring this part of the code to scrap it without breaking my code?
  • My other question is on syntax. Further along down my code I have helper classes that use some variables. I've read that when you use variables with helper functions you should just declare those variables right above the function. Is that correct and am I using that methodology correctly?
  • Finally if there is something you see that should be done another way because of some easier way to do things please let me know.

public class PlayerMovement : MonoBehaviour {
    MeshRenderer meshRenderer;
    [SerializeField] bool usingTouch;
    Vector3 force;
    ShipManager shipManager;
    float topSpeed = 20;

    public Canvas Canvas;
    DebugController dc;
    bool isBeingTouched = false;
    int beingTouchedByFingerNumber = -1;
    float zPos;
    float distFromCamera;

    MotionState motionState;
    enum MotionState {
        waiting,
        start,
        drag,
    }

    public void Start() {
        meshRenderer = GetComponent<MeshRenderer>();
        shipManager = GetComponent<ShipManager>();
        dc = Canvas.GetComponent<DebugController>();
        zPos = gameObject.transform.position.z;
        GameObject camera = GameObject.Find("Main Camera");
        distFromCamera = transform.position.z - camera.transform.position.z;//Used for ScreenToWorldPoint()
    }

    public LayerMask touchInputMask;
    RaycastHit hit;
    void Update() {
        if (Input.touchCount > 0) {
            foreach (Touch touch in Input.touches) {
                Ray ray = Camera.main.ScreenPointToRay(touch.position);

            if (Physics.Raycast(ray, out hit, touchInputMask)) {
                GameObject recipient = hit.transform.gameObject;
                if (recipient.tag == "Player" && !isBeingTouched) {
                    OnTouchEnter(touch);
                }
            }
        }
    }
        if (beingTouchedByFingerNumber != -1) {
            dc.ChangeText(7, "Touched by Phase: " + Input.GetTouch(beingTouchedByFingerNumber).phase.ToString());
            dc.ChangeText(8, motionState.ToString());
        }
        if (beingTouchedByFingerNumber != -1 && Input.GetTouch(beingTouchedByFingerNumber).phase == TouchPhase.Ended) {
            OnTouchUp();
        }
        HandleMovement();
    }

    private void HandleMovement() {
        switch (motionState) {
            case MotionState.start:
                break;
            case MotionState.drag:
                Vector2 v2 = (Input.GetTouch(beingTouchedByFingerNumber).position);
                handlePlayerDrag(v2);
                break;
            case MotionState.waiting:
                break;
            default:
                break;
        }
    }

    public void FixedUpdate() {
        if (motionState == MotionState.waiting)
            handleVelocity();
    }

    private void OnTouchEnter(Touch touch) {
        beingTouchedByFingerNumber = touch.fingerId;
        isBeingTouched = true;
        shipManager.GetComponent<Rigidbody>().isKinematic = true;
        Vector2 v2 = Input.GetTouch(beingTouchedByFingerNumber).position;
        GetOffsetOfMouseFromShip(v2);
        motionState = MotionState.drag;
        meshRenderer.enabled = false;
    }

    public void OnTouchEnter() {
        isBeingTouched = true;
        shipManager.GetComponent<Rigidbody>().isKinematic = true;
        Vector2 v2 = Input.GetTouch(beingTouchedByFingerNumber).position;
        GetOffsetOfMouseFromShip(v2);
        motionState = MotionState.drag;
        meshRenderer.enabled = false;
    }

    public void OnTouchUp() {
        isBeingTouched = false;
        shipManager.GetComponent<Rigidbody>().isKinematic = false;
        handleForceAveraging();
        beingTouchedByFingerNumber = -1;
        motionState = MotionState.waiting;
        meshRenderer.enabled = true;
    }

    public void handlePlayerDrag(Vector2 v2) {
        float step = topSpeed * Time.deltaTime;
        Vector3 moveTo = turnTouchInputToWorldPoint(v2);
        transform.position = Vector3.MoveTowards(transform.position, Camera.main.ScreenToWorldPoint(moveTo) + offset, step);
        force = moveTo - previousLocation;//Changes the force to be applied
        previousLocation = moveTo;
        forceAddedToVectorArray();
    }

    //Turns a Vector2 touchInput to a world point
    private Vector3 turnTouchInputToWorldPoint(Vector2 v2) {
        Vector3 v3;
        if (usingTouch) {
            v3 = new Vector3(v2.x, v2.y, distFromCamera);
        } else {
            v3 = new Vector3(Input.mousePosition.x, Input.mousePosition.y, gameObjectToScreenPoint.z);
        }
        return v3;
    }

    private void handleVelocity() {
        shipManager.GetComponent<Rigidbody>().velocity = force;
    }

    Vector3 gameObjectToScreenPoint;
    Vector3 previousLocation;
    Vector3 offset;
    public void GetOffsetOfMouseFromShip(Vector2 v2) {
        //This grabs the position of the object in the world and turns it into the postion on the screen
        gameObjectToScreenPoint = Camera.main.WorldToScreenPoint(gameObject.transform.position);
        Vector3 v3;
        if (usingTouch) {
            v3 = new Vector3(v2.x, v2.y, gameObjectToScreenPoint.z);
        } else {
            v3 = new Vector3(Input.mousePosition.x, Input.mousePosition.y, gameObjectToScreenPoint.z);
        }
        //Sets the mouse pointers vector3
        previousLocation = v3;
        //Set offset of mouse and ship
        offset = gameObject.transform.position - Camera.main.ScreenToWorldPoint(previousLocation);
    }

    //Add the force to a vector array to get the average of force to later apply on release of the ship
    public Vector3[] forces = new Vector3[10];
    public Vector3 forceAverage = Vector3.zero;
    public int forcesArrayLocation = 0;
    private void forceAddedToVectorArray() {
        forces[forcesArrayLocation] = force;
        forcesArrayLocation++;
        if (forcesArrayLocation == forces.Length)
            forcesArrayLocation = 0;
    }

    private void handleForceAveraging() {
        for (int i = 0; i < forces.Length; i++) {
            forceAverage += forces[i];
        }
        forceAverage /= forces.Length;
        force = forceAverage;
        //Makes sure there isn't a ludicrous speed
        if (force.sqrMagnitude > topSpeed * topSpeed) {
            force = force.normalized * topSpeed;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

The first thing is I have an embedded class of MotionState. At first glance it makes a lot of sense to start with that. I latter ended up not using it. How do I go about refactoring this part of the code to scrap it without breaking my code?

First, MotionState is not a class but an enum. Setting this aside, you say that you are not using it, but you have used it 7 times in the current code.


My other question is on syntax. Further along down my code I have helper classes that use some variables. I've read that when you use variables with helper functions you should just declare those variables right above the function. Is that correct and am I using that methodology correctly?

These classes aren't contained in the question so it is a little bit hard to answer but I try it nevertheless.

You should always declare your variables as near as possible to their usage. If a variable is only used in one method it should be declared inside this method to give it the most limited scope.


Finally if there is something you see that should be done another way because of some easier way to do things please let me know.

  • your indention is completely off. If you are using Visual Studio with the C# key binding, the default keyboard shortcuts are Ctrl + E,D to format the document or Ctrl + E,F to format the selected code.

  • you should stick to the style you have choosen.
    Right now you have sometimes stated explicitly the scope of methods private sometimes you left it to default which also is private.
    Sometimes you use braces {} for single statement if sometimes you don't.

    I would like to encourage you to use them always which helps to make your code less error prone.

  • Comments should describe why something is done in the way it is done. Let the code speak for itself what is done by using meaningful names for variables, methods and classes.

  • Based on the naming guidelines methods should be named using PascalCase casing. Right now your are mixing PascalCase and camelCase casing for naming your methods.

  • Shortening variable names like DebugController dc; reduces the readability of the code. You should avoid using abbreviations.

  • you have some public variables in your class. This open up the class for beeing manipulated from the outside which can lead to unecpected behaviour.

    If you need to read the values from outside of the class you should turn them into properties with a public getter and a private setter.

Now let us dig into your code

//Turns a Vector2 touchInput to a world point
private Vector3 turnTouchInputToWorldPoint(Vector2 v2) {
    Vector3 v3;
    if (usingTouch) {
        v3 = new Vector3(v2.x, v2.y, distFromCamera);
    } else {
        v3 = new Vector3(Input.mousePosition.x, Input.mousePosition.y, gameObjectToScreenPoint.z);
    }
    return v3;
}

could be written like

private Vector3 TurnTouchInputToWorldPoint(Vector2 v2) {

    if (usingTouch) {
        return new Vector3(v2.x, v2.y, distFromCamera);
    } 

    return new Vector3(Input.mousePosition.x, Input.mousePosition.y, gameObjectToScreenPoint.z);
}  

btw, I just noticed you are using the K&R style for placing your braces instead of the Allman style. If this is written in your coding guidelines you should stick to it, if not it would be better to switch to Allman because most C# developer uses it. In this way a new developer can easier read your code.


private void OnTouchEnter(Touch touch) {
    beingTouchedByFingerNumber = touch.fingerId;
    isBeingTouched = true;
    shipManager.GetComponent<Rigidbody>().isKinematic = true;
    Vector2 v2 = Input.GetTouch(beingTouchedByFingerNumber).position;
    GetOffsetOfMouseFromShip(v2);
    motionState = MotionState.drag;
    meshRenderer.enabled = false;
}

public void OnTouchEnter() {
    isBeingTouched = true;
    shipManager.GetComponent<Rigidbody>().isKinematic = true;
    Vector2 v2 = Input.GetTouch(beingTouchedByFingerNumber).position;
    GetOffsetOfMouseFromShip(v2);
    motionState = MotionState.drag;
    meshRenderer.enabled = false;
}

Here the first method could be made smaller by calling the second method after using the Touch parameter like so

private void OnTouchEnter(Touch touch) {
    beingTouchedByFingerNumber = touch.fingerId;
    OnTouchEnter();
}

private void HandleMovement() {
    switch (motionState) {
        case MotionState.start:
            break;
        case MotionState.drag:
            Vector2 v2 = (Input.GetTouch(beingTouchedByFingerNumber).position);
            handlePlayerDrag(v2);
            break;
        case MotionState.waiting:
            break;
        default:
            break;
    }
}  

by replacing the switch with a simple if like so

private void HandleMovement() {
    if (motionState != MotionState.drag) { return; }

    Vector2 v2 = (Input.GetTouch(beingTouchedByFingerNumber).position);
    handlePlayerDrag(v2);
}

the code can be reduced a lot.


public LayerMask touchInputMask;
RaycastHit hit;
void Update() {
    if (Input.touchCount > 0) {
        foreach (Touch touch in Input.touches) {
            Ray ray = Camera.main.ScreenPointToRay(touch.position);

        if (Physics.Raycast(ray, out hit, touchInputMask)) {
            GameObject recipient = hit.transform.gameObject;
            if (recipient.tag == "Player" && !isBeingTouched) {
                OnTouchEnter(touch);
            }
        }
    }
}
    if (beingTouchedByFingerNumber != -1) {
        dc.ChangeText(7, "Touched by Phase: " + Input.GetTouch(beingTouchedByFingerNumber).phase.ToString());
        dc.ChangeText(8, motionState.ToString());
    }
    if (beingTouchedByFingerNumber != -1 && Input.GetTouch(beingTouchedByFingerNumber).phase == TouchPhase.Ended) {
        OnTouchUp();
    }
    HandleMovement();
}

this is a mess in the meaning of indention, so let us first fix it like so

public LayerMask touchInputMask;
RaycastHit hit;
void Update()
{
    if (Input.touchCount > 0)
    {
        foreach (Touch touch in Input.touches)
        {
            Ray ray = Camera.main.ScreenPointToRay(touch.position);

            if (Physics.Raycast(ray, out hit, touchInputMask))
            {
                GameObject recipient = hit.transform.gameObject;
                if (recipient.tag == "Player" && !isBeingTouched)
                {
                    OnTouchEnter(touch);
                }
            }
        }
    }
    if (beingTouchedByFingerNumber != -1)
    {
        dc.ChangeText(7, "Touched by Phase: " + Input.GetTouch(beingTouchedByFingerNumber).phase.ToString());
        dc.ChangeText(8, motionState.ToString());
    }
    if (beingTouchedByFingerNumber != -1 && Input.GetTouch(beingTouchedByFingerNumber).phase == TouchPhase.Ended)
    {
        OnTouchUp();
    }
    HandleMovement();
}

as you will see it now uses the Allman style, because thats how my IDE is configured.

These if statements should be grouped together, so you don't need to check twice for beingTouchedByFingerNumber like so

    if (beingTouchedByFingerNumber != -1)
    {
        dc.ChangeText(7, "Touched by Phase: " + Input.GetTouch(beingTouchedByFingerNumber).phase.ToString());
        dc.ChangeText(8, motionState.ToString());
        if (Input.GetTouch(beingTouchedByFingerNumber).phase == TouchPhase.Ended)) 
        {
            OnTouchUp();
        }
    }

If Input.touchCount is the same as the number of items in Input.touches you can refactor

    if (Input.touchCount > 0)
    {
        foreach (Touch touch in Input.touches)
        {
            Ray ray = Camera.main.ScreenPointToRay(touch.position);

            if (Physics.Raycast(ray, out hit, touchInputMask))
            {
                GameObject recipient = hit.transform.gameObject;
                if (recipient.tag == "Player" && !isBeingTouched)
                {
                    OnTouchEnter(touch);
                }
            }
        }
    }  

like so

foreach (Touch touch in Input.touches)
{
    Ray ray = Camera.main.ScreenPointToRay(touch.position);

    if (!Physics.Raycast(ray, out hit, touchInputMask)) { continue; }

    GameObject recipient = hit.transform.gameObject;
    if (recipient.tag == "Player" && !isBeingTouched)
    {
        OnTouchEnter(touch);
    }
}

but wait, what should we do with these 2 variables outside of the method, which are only used inside of this method ?

The public one we will keep outside because I guess you have a reason for it. But the other should be declared as near as possible to its usage.

The method would then look like so

private void Update()
{
    foreach (Touch touch in Input.touches)
    {
        Ray ray = Camera.main.ScreenPointToRay(touch.position);
        RaycastHit hit;
        if (!Physics.Raycast(ray, out hit, touchInputMask)) { continue; }

        GameObject recipient = hit.transform.gameObject;
        if (recipient.tag == "Player" && !isBeingTouched)
        {
            OnTouchEnter(touch);
        }
    }

    if (beingTouchedByFingerNumber != -1)
    {
        dc.ChangeText(7, "Touched by Phase: " + Input.GetTouch(beingTouchedByFingerNumber).phase.ToString());
        dc.ChangeText(8, motionState.ToString());
        if (Input.GetTouch(beingTouchedByFingerNumber).phase == TouchPhase.Ended)) 
        {
            OnTouchUp();
        }
    }  

    HandleMovement();

}

Finished for now, leaving the remaining for you.

\$\endgroup\$
  • 1
    \$\begingroup\$ Public fields are required in Unity3D for the inspector to properly serialize them. There is a [SerializeField] attribute which also works, but this is an uncommon solution. \$\endgroup\$ – Nick Udell Jul 9 '15 at 12:43
  • \$\begingroup\$ The Visual Studio shortcut chord on all of my installations (by default) is CTRL + K, CTRL + D. \$\endgroup\$ – Der Kommissar Jul 9 '15 at 14:26
  • \$\begingroup\$ @Heslacher 2012 Ultimate, 2013 Ultimate, and 2015 Enterprise RC. \$\endgroup\$ – Der Kommissar Jul 9 '15 at 14:28
  • 1
    \$\begingroup\$ Seems it depends on the key bindings. See: stackoverflow.com/a/4942145/2655508 \$\endgroup\$ – Heslacher Jul 9 '15 at 14:31
  • \$\begingroup\$ @Heslacher Aha, perhaps you should include that in this answer as well? \$\endgroup\$ – Der Kommissar Jul 9 '15 at 14:33

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.