「僕Unityできますよ」Tシャツのコードレビュー

検証環境 : Unity 2017.3.1p4
検証日: 2018/04/23

＊本記事は「ネタにマジレス」しています

「僕Unityできますよ」Tシャツとは

TwitterでUnity使いの間で話題になったジョークTシャツです

何がジョークなの？

Tシャツに「僕Unityできますよ」という文字とソースコードが書かれているのですが、
そのコードがいわゆる「クソコード」と呼ばれるものなのです

どこがクソなの？

まず、Tシャツのソースコードは以下の通りです

public float speed = 100;

	void Update () 
    {
        if(GameObject.Find("MainGame").GetComponent<MainGame>().gameStart)
        {
            if(Input.GetButton("Vertical"))
            {
                transform.position += transform.forward * speed;
            }
        }
	}

このコードですが、何が一番問題なのかと言うと、なんとジョークTシャツなのにコンパイルがほぼ通ってしまうことです
（MainGameにgameStartのboolがあれば通ってしまいます。詳しくは後述）

コンパイル通るのですが、これだけのコードにUnityで言われるバッドノウハウが詰まっています。
本記事ではそれにマジレスをしていきます。
あくまで個人のレビューですので、さらなるツッコミや指摘は大歓迎です。

1.publicな変数に初期値をつけるのは気をつける

まずは１行目です

public float speed = 100;

一見ただ変数の定義に見えますが、ことUnityにおいては危険なポイントの一つです。
一度この値の状態でオブジェクトにスクリプトがアタッチされてしまっていると、
Inspector上に値が100で登録され、
ソースコードの値を100から10に変えたとしても、その情報は既存のオブジェクトに反映されません

しかしながら、これは一概に悪いこととは言えません。
同じスクリプトで、Speedだけ変えたい場合が多々あるからです。
そういった場合は少し難しい話になるかもしれませんが、以下のようにInspectorで値を設定するものだと分かりやすくするといいでしょう

[SerializeField]
private float speed = 100;

2.Update()でFindやGetComponentを呼ばない

ここがUnityチョットデキル人が一番突っ込みたくなる部分でしょう。

 if(GameObject.Find("MainGame").GetComponent<MainGame>().gameStart)

＊　MainGameにはおそらくpublic bool gameStartがあることでしょう。

このコードはMainGameと言う名前のオブジェクトがあり、さらにそれがMainGameと言うコンポーネントが付いているとします。
しかし、FindやGetComponentはUnityの中でもかなり重たい処理です。

これを毎フレーム実行することはUnityのマニュアルでも非推奨となっています
https://docs.unity3d.com/ja/current/ScriptReference/GameObject.Find.html

なので、こういったコードを書く場合はMainGameをキャッシュする、もしくはシングルトンにしておくとパフォーマンスがよくなります。

3.前に進むことしか出来ない

こちらは仕様による部分も大きいですが

            if(Input.GetButton("Vertical"))
            {
                transform.position += transform.forward * speed;
            }

このコード、Vertical、つまり↑矢印やwキーを押すことで処理が入るようになっています。
実際に動かす部分は後述しますが、この実装では
↓矢印やsキーを押しても前に進みます
なので、positionに値を加える際はVerticalの値を参照し、
Input.GetAxis("Vertical")
をspeedにかけるか、前にしか進まないのが仕様なら、↓やsの入力（Vertical <= 0)の時は何もしないようにした方がいいでしょう。
また、Time.deltaTimeをかけてあげることでフレーム落ちなどによる差を軽減できます。

4.1 transformの移動方法はこれでいい？

ここからは少し難しい話になりますが、
Unityには数々のオブジェクトの移動方法があります。

• transform.Translate
• transform.position +=
• transform.position =
• Rigidbody.MovePosition
• Rigidbody.AddForce
• RigidBody.velocity

などです。この中での私のオススメは、Rigidbody.velocity、もしくはRigidbody.AddForceです

4.2 なぜRigidbody.velocityがいい？

まずこのUnity出来ますよコードが、FPSやアクションゲームのコードっぽいのでそうであると仮定します。
そうした場合、transform.positionやRigidbody.MovePositionでは「壁の中にいる」状態や「壁抜け」が発生しやすくなります。

図では分かりやすく、Rigidbody.velocityの値を10000にしています。
transform.Translateでは青いブロックがどこかに飛んでいってしまいますが、Rigidbody.velocityではちゃんと壁に阻まれてくれます。
これはtransform.Translateなどがオブジェクトのテレポートなのに対し、Rigidbody.velocityは物理演算で動いているからです。
（＊値を1000000などにするとvelocityでも壁を抜けます）

また、　speedの値が小さい場合どうなるかは次の図をご覧ください

transform.Translateの方は壁にあたり、キーを離したあとあらぬ方向に吹っ飛びます。
これは一瞬壁の中に入ったりすることにより物理演算がおかしくなってしまうからです。
なので、物理シミュレートの観点からもTranslateなどはおすすめしません。
なのでこの部分はこう書くといいでしょう(1行に納めるためにGetComponentを使っていますが、前述の通りよくないです)

GetComponent<Rigidbody>().velocity = Vector3.forward * Time.deltaTime * speed;

4.3 移動系はそもそもFixedUpdateで

実は順番が前後してしまいますが、物理演算系の移動をする際はFixedUpdateを使うと幸せになれます。
4.2のRigidbody.velocityではUpdate()では壁抜けの危険性があります。

これは物理演算がFixedUpdateの更新タイミングで呼ばれるため、Updateの中でvelocityをいじると動作が安定しないためです。
FixedUpdateは0.02秒に１回呼ばれるのに対し、Updateが呼ばれるタイミングは不規則なためです。
ここでは詳細は述べませんが、物理系はFixedUpdateで操作する。と覚えれば大丈夫でしょう。

4.4 入力はUpdateで

僕UnityできますTシャツの話から少し外れますが、FixedUpdateでは
Input系の処理はFixedUpdate内で書かない方が無難です。
理由を簡単に言うと、入力のイベントはフレーム単位で呼ばれているので、
FixedUpdateのタイミングでは入力情報が間に合わなくなっていることがあるからです。

なので、以下のようにして入力を安全にする必要があります

private bool isPress = false;
    void Update(){
         if(Input.GetButton("Vertical")){
             isPress = true;
         }
    }
    void FixedUpdate(){
       if(isPress){
           GetComponent<Rigidbody>().velocity = Vector3.forward * speed;
           isPress = false;
        }
    }

（4/24追記） Time.deltaTimeはFixedUpdate上では不必要なため、こちらのコードからは除外しました。
前述した通り呼ばれるタイミングが一定であるためです
以上です。指摘などありましたらコメントしていただけると励みになります。