WaypointsMover review

Review.md

Отступы

Не нужно смешивать табы с пробелами. Конкретно для тебя проблема решается автозаменой таба на 4 пробела по всему коду. Также стоит настроить редактор, чтобы он заменял табы на пробелы при вводе.

Функциональность

Класс реализует две пересекающиеся функциональности. Возможно, имеет смысл разделить на два? Стоит помнить, что разделение, хоть и упрощает каждый класс в отдельности, всю систему может усложнить или хотя бы банально привести к большему количеству кода.

Альтернатива разделению - обособление частей кода отвечающих за линейное и за криволинейное движение. Сейчас это сделано для методов, но остаются аттрибуты, вызовы этих методов и хак в конструкторе.

Ещё одна альтернатива - вынести логику кривизны в объект-стратегию, в данном случае можно использовать Path. Т.е. создаем два класса CurvedPath и LinearPath, которые и будут считать позицию в зависимости от времени.

Кроме того:

• неконсистентно использование пустых строк,
• нарушаются соглашения о наименовании (хотя с этим в Unity/.NET вообще проблема, как я понял),
• используются сокращения в именах,
• присутствует код по логике принадлежащий к другому классу,
• некоторые методы избыточны.

Почему не использовать сокращения:

• можно написать по-разному,
• читающему код может быть непонятно (что такое splineT?)

Более подробно в коде, в комментариях начинающихся с // R:

ReviewIndentation.png

WaypointsMover.cs

using System;
using System.Collections;
using System.Security.Cryptography;
using UnityEngine;
using Random = UnityEngine.Random;

namespace Assets.Plugins.Smart2DWaypoints.Scripts
{
    public class WaypointsMover : MonoBehaviour
    {
        // R: ты нарушаешь соглашения об именовании и для .NET и для Unity:
        //    - используешь подчёркивание - https://msdn.microsoft.com/en-us/library/vstudio/ms229045(v=vs.100).aspx
        //    - по конвенции Unity свойства должны начинаться со строчной буквы,
        //      правда по конвенции .NET с заглавной :)
        // R: непонятно отбитие пустыми строками. Почему иногда 1 иногда 2?
        // R: непонятно по каким принципам сгруппированы объявления,
        //    можно было бы группировать по принципу совместного использования,
        //    например, если бы _splineCurve, _splineT и _partTime шли вместе,
        //    то было бы сразу ясно, что _partTime относится к сплайнам.
        //    Кроме того, перед группой можно было бы написать коммент:
        //
        //        // Curve handling
        //        private float _splineT;
        //        private float _partTime;
        //        private bool _isRunning;
        //
        //    Или можно назвать переменные так, чтобы у них был общий знаменатель:
        //
        //        private SplineCurve splineCurve;
        //        private float curveProgress;
        //        private float curveTime;
        //
        //    В этом случае комментарий не нужен, он смотрелся бы просто глупо.
        //    Кстати, в качестве общего знаменателя я выбрал curve, а не spline,
        //    т.к. сплайны - это деталь реализации, а работаем мы с кривыми.
        //    Curve также следует использовать в названии методов (уже сделано)
        protected Transform Transform;
        protected int _targetInd; // R: лучше не использовать сокращения, targetIndex
        private SplineCurve _splineCurve;
        private float _splineT; // R: опять сокращение, что такое _splineT?
        private float _partTime;
        private bool _isRunning;
        private bool _isForwardDirection; // R: isForwardDirection = является прямым направлением
                                          //    isGoingForward не только лучше звучит,
                                          //    но и согласуется с isRunning
        private Vector3 _prevPosition;

        private float _directionMagnitudeLimit; // R: неясно

        // R: именованные константы лучше вынести на самый верх,
        //    так человек, открывающий файл, сразу увидит, что здесь можно настроить.
        public const float InitSpeed = 0.139139f;


        public Path Path;
        public float Speed = InitSpeed;
        public Transform StartWaypoint;
        public bool IsRandomStartWaypoint;
        public LoopType LoopType = LoopType.Looped;
        public bool IsAlignToDirection; // R: более по английски будет IsAlignedToDirection,
                                        //    ещё более - shouldAlignToDirection,
                                        //    но нарушает соглашение "флажки начинаются с is"
        public float RotationOffset;
        public bool IsXFlipEnabled;
        public bool IsYFlipEnabled;

        public void Start()
        {
            Transform = transform; // R: откуда пришёл этот transform?
            _isForwardDirection = true;
            _isRunning = true;
            if (Path.IsCurved)
                _splineCurve = new SplineCurve();
            InitStartWaypoint();
            // R: магическое число
            _directionMagnitudeLimit = 0.0002f * Camera.main.orthographicSize;
        }

        private void InitStartWaypoint()
        {
            if (StartWaypoint != null)
            {
                _targetInd = Mathf.Max(0, Path.Waypoints.IndexOf(StartWaypoint));
            }
            else if (IsRandomStartWaypoint)
            {
                _targetInd = Random.Range(0, Path.Waypoints.Count);
            }
            Transform.position = Path.Waypoints[_targetInd].position;
            StartCoroutine(OnWaypointReached(TakeNextWaypointOnStart));
        }

        public virtual void Update()
        {
            // R: почему нельзя просто написать код здесь?
            OnUpdate();
        }

        // R: неожиданное новое приватное поле
        private Vector3 Direction;

        protected void OnUpdate()
        {
            if (_isRunning)
            {
                UpdateDirection();

                EnsureAlignToDirection();

                _prevPosition = Transform.position;
                if (Path.IsCurved)
                    UpdateCurved();
                else
                    UpdateLinear();

                UpdateFlips();
            }
        }

        private void UpdateDirection()
        {
            // R: почему не "if (!Path.IsCurved)" ?
            if (Path.IsCurved)
                ;
            else
                Direction = GetDirectionFromLinear();
        }

        protected virtual void EnsureAlignToDirection()
        {
            if (IsAlignToDirection)
            {
                // R: правильнее текущий поворот получать из Path и прогресса на текущем сегменте
                Vector2 direction = Transform.position - _prevPosition;
                if (direction.magnitude > _directionMagnitudeLimit)
                {
                    float angle = direction.y > 0
                        ? Vector2.Angle(new Vector2(1, 0), direction)
                        : -Vector2.Angle(new Vector2(1, 0), direction);
                    RotateTo(angle + RotationOffset);
               }
            }
        }

        protected virtual void RotateTo(float angle)
        {
            Transform.rotation = Quaternion.identity;
            Transform.Rotate(Vector3.forward, angle);
        }

        private void TakeNextWaypointOnStart()
        {
            _targetInd++;
        }

        protected void TakeNextWaypoint()
        {
            // R: этот метод тавтологичен, лучше в данном случае не создавать по методу на исход,
            //    а расписать всё здесь, позаботившись, чтобы куски кода были небольшими.
            switch (LoopType)
            {
                case LoopType.OneWay:
                {
                    TakeNextWaypointForOneWay();
                    break;
                }
                case LoopType.Looped:
                {
                    TakeNextWaypointForLooped();
                    break;
                }
                case LoopType.PingPong:
                {
                    TakeNextWaypointForPingPong();
                    break;
                }
            }
        }

        // R: порядок этих 3 функций не соответствует порядку в switch,
        //    а вообще, порядок должен определятся enum-ом
        private void TakeNextWaypointForOneWay()
        {
            // R: нехорошо, что эта функция, кроме того, что заявлено в названии,
            //    ещё и isRunning переключает
            // R: условие должно быть методом Path
            //      if (Path.IsEnd(_targetInd)) { ... }
            if ((Path.IsClosed && _targetInd == 0) ||
                (!Path.IsClosed && _targetInd == Path.Waypoints.Count - 1))
                OnOneWayFinished();
            _targetInd = (_targetInd + 1)%Path.Waypoints.Count;
        }

        private void TakeNextWaypointForPingPong()
        {
            // R: if (Path.isStart(_targetInd) || Path.isEnd(_targetInd)) { ... }
            if (_targetInd == 0 ||
                (!Path.IsClosed && _targetInd == Path.Waypoints.Count - 1))
                _isForwardDirection = !_isForwardDirection;

            // R: можно тоже унести в Path:
            //      _targetInd = Path.NextIndex(_targetInd, _isForwardDirection)
            int dInd = _isForwardDirection ? 1 : -1;
            _targetInd = (_targetInd + dInd + Path.Waypoints.Count)%Path.Waypoints.Count;
        }

        private void TakeNextWaypointForLooped()
        {
            if (!Path.IsClosed && _targetInd == Path.Waypoints.Count - 1)
            {
                Transform.position = Path.Waypoints[0].position;
                _targetInd = 0;
            }
            else
            {
                _targetInd = (_targetInd + 1)%Path.Waypoints.Count;
            }
        }

        // R: эта функция не делает код более понятным, можно было просто заинлайнить строчку
        protected virtual void OnOneWayFinished()
        {
            _isRunning = false;
        }

        protected IEnumerator OnWaypointReached(Action nextAction)
        {
            Transform waypoint = Path.Waypoints[_targetInd];
            DelayWaypoint delayWaypoint = waypoint.GetComponent<DelayWaypoint>();
            // R: более логично написать:
            //    if (delayWaypoint) {
            //        _isRunning = false;
            //        yield ...
            //        ...
            //    }
            //    RunNextAction(nextAction);
            if (delayWaypoint == null)
            {
                RunNextAction(nextAction);
            }
            else
            {
                _isRunning = false;
                yield return new WaitForSeconds(delayWaypoint.Delay);
                _isRunning = true;
                RunNextAction(nextAction);
            }
        }

        private void RunNextAction(Action nextAction)
        {
            nextAction();

            if (Path.IsCurved)
            {
                if (!Path.IsClosed && LoopType == LoopType.Looped && _targetInd == 0)
                    StartCoroutine(OnWaypointReached(nextAction));
                else
                    UpdateCurvePoints();
            }
        }

        #region Linear

        public Vector3 GetDirectionFromLinear()
        {
            return (Path.Waypoints[_targetInd].position - Transform.position).normalized;
        }

        protected virtual void UpdateLinear()
        {
            Transform waypoint = Path.Waypoints[_targetInd];
            // R: правильнее будет считать не от текущей позиции, а от начала сегмента,
            //    и интерполировать линейно соответственно пройденному времени.
            //    Повышается точность и подход совпадает со сплайновым.
            //    Доп. бонусы:
            //      - не нужно хранить Direction
            //      - не нужен метод UpdateDirection
            //      - не нужен метод GetDirectionFromLinear
            Vector3 nextPosition = Transform.position + Direction * Speed * Time.deltaTime;
            if (Vector3.Distance(Transform.position, waypoint.position) <=
                Vector3.Distance(Transform.position, nextPosition))
            {
                Transform.position = waypoint.position;
                StartCoroutine(OnWaypointReached(TakeNextWaypoint));
            }
            else
            {
                Transform.position = nextPosition;
            }
        }

        #endregion

        #region Curved
        private void UpdateCurved()
        {
            // R: логику начисления времени нужно отделить,
            //    код должен упроститься если мы будем работать по такому алгоритму:
            //        - этот класс ведёт учет расстояния, а не времени,
            //        - по расстоянию определяется сегмент,
            //        - по сегменту и остатку расстояния определяется позиция.
            //    таким образом, текущая позиция - это просто функция пройденного расстояния,
            //    не нужно следить отдельно за временем, текущим сегментом, временем на сегмент,
            //    частью пройденного сплайна.
            _splineT += Time.deltaTime / _partTime;
            if (_splineT > 1.0f)
            {
                // R: по идее мы должны проскочить и уже маленько начать двигаться
                //    по следующему сегменту
                _splineT = 0.0f;
                StartCoroutine(OnWaypointReached(TakeNextWaypoint));
            }
            else
            {
                MoveCurved(_splineCurve.GetPoint(_splineT));
            }
        }

        protected virtual void MoveCurved(Vector3 point)
        {
            Transform.position = point;
        }

        private void UpdateCurvePoints()
        {
            int dInd = _isForwardDirection ? 1 : -1;
            int baseInd = (_targetInd - dInd + Path.Waypoints.Count)%Path.Waypoints.Count;
            int[] indexes = Path.GetSplinePointIndexes(baseInd, _isForwardDirection);
            _splineCurve.ApplyPoints(
                Path.Waypoints[indexes[0]].transform.position,
                Path.Waypoints[indexes[1]].transform.position,
                Path.Waypoints[indexes[2]].transform.position,
                Path.Waypoints[indexes[3]].transform.position);
            _partTime = Path.GetLength(baseInd)/Speed;
        }

        #endregion

        #region Flips

        private void UpdateFlips()
        {
            if (IsXFlipEnabled && Transform.localScale.x*Direction.x < 0)
                Transform.localScale = new Vector3(-Transform.localScale.x, Transform.localScale.y);

            if (IsYFlipEnabled && Transform.localScale.y*Direction.y < 0)
                Transform.localScale = new Vector3(Transform.localScale.x, -Transform.localScale.y);
        }

        #endregion

    }
}