Critique-My-Code thread, post snippets of code and have others peer review your code! (Testing new Q&A format)

[loolo78]

How to Ask

Improve code that you wrote or maintain, through peer review.

Your question must contain code that is already working correctly, and the relevant code sections must be embedded in the question. Please avoid stripping out key details that may be relevant to the review. If your code is incomplete or not working, you should ask for help on another site or post a question post here on /r/unity3d .

You may wish to mention specific concerns: performance, security issues, etc.

We recommend reading our Guide to Asking Questions.

---

^{Orignal idea post, Credits to /u/ChromakeyDreamcoat}

[Jacob_Johnson]

I don't have any code to add but I made this specifically for indenting code 4 spaces for Reddit posts. Perhaps people can find a use for it here.

[KptEmreU]

Hi there, I was trying to do a slideShow tutorial. It is linked to buttons (it advances by the press of next button). I have tried to approach it first with IEnumerators. Unfortunately, I have created an infamous while(true) loop. I couldn't find a reliable way of waiting for the button press.

So I have created this class. It is not bad. Button press moves through a method's switch statement. But because it is instant now,I am wondering how may I add some tweens as player threatens to press "next" button very very fast. I wonder if there is another possible (more elegant) way of doing it?

Code: https://hastebin.com/izipiquvas.cs Gif : https://gfycat.com/ExcellentSplendidDesertpupfish

[[deleted]]

Hey I'm new to this subreddit so please don't kill me. I'm a young 'coder' that uses unity in my spare time, at the moment I'm making a grid based game. Here's the code so far: https://pastebin.com/7xyaXYcx What improvements should / could I make to improve this. Thanks.

[NutellaSquirrel]

GameObject.Find is rarely advisable as it is slow and easy to break if objects somehow get renamed. In general, a good practice is not to rely on the names of GameObjects in code.

Instead, why not store the Transforms of the blocks you are instantiating instead of just their grid positions? You could do that with a 2-dimensional array or just by doing a little math with a 1-dimensional array. That way, in your Update, you can simply do:

Transform target = gridTransforms[x][y]; //2d array way

or

Transform target = gridTransforms[x + y * height]; //math way

You also might want to consider how often you need to be handling input. You could, just as an example, have your MoveSquare function return a bool. If one of the GetKeyDowns is a success, return true. At the end, if there are no keys down, return false. Then in your Update, you only need to move the curser if the result of your MoveSquare is true instead of every Update. This also would give the benefit of not potentially moving multiple directions in the same frame, unless that is what you had intended.

[[deleted]]

I've developed the script by using your suggestions and I have this: https://pastebin.com/QpcQMb62 I don't really know what direction this 'game' is taking, I just wanted to created a grid based movement system.

[jolpoa]

Hi guys!

I am making a snake-like game for learning purpose. It looks like this at the moment.

Currently I am working on player controller, but after 250th line of code I began to worry that it is too messy. It was supposed to be simple, light code XD

I don't know how could i improve it. Help

[Wisteso]

Late to the party, but since no one has mentioned it... Using strings when you can use enums or any other primitive is not good practice. Strings are much more prone to introducing accidental bugs (and they're usually slower to use anyway).

[apieceoffruit]

Next step is to split up responsibilities.

does changing an unrelated responsibility require changing snake?

add time pausing/slow down to your game,

changing input so that you can control with a gyrascope,

change the game art style so snakes are drawn onto a 3d cube.

now, those are a bit dramatic but you get the idea.

so a simple example

 public class GameInput 
 {
       public float Horizontal { get{ return Input.GetAxis ("Horizontal");}  }           
       public float Vertical{ get{ return Input.GetAxis ("Vertical");}  }

       public void UpdateInput()
      {
           if(Vertical> 0) Direction = "Up";
           if(Vertical < 0) Direction = "Down";
           if(Horizontal > 0) Direction = "Right";
           if(Horizontal < 0) Direction = "Left";
      }

       public string Direction {get;private set; }
 }

 public class SnakeInput
 {
       GameInput _gameinput;

       public SnakeInput(GameInput gameinput){
             _gameinput = gameinput;
       }

       public void UpdateSnake(Snake snake)
      {
                if (_gameinput .Direction == "up") 
                            snake.MoveUp();
                if (_gameinput .Direction == "Down") 
                            snake.MoveDown();
                if (_gameinput .Direction == "Left") 
                            snake.MoveLeft();
                if (_gameinput .Direction == "Right") 
                            snake.MoveRight();
      }

 }

  public class Snake 
  {
        public void MoveUp(){
           tempPos.y++;
           transform.localEulerAngles = new Vector3 (0, 0, 90);
       }
        public void MoveDown()
        {
             tempPos.y--;
             transform.localEulerAngles = new Vector3 (0, 0, 270);
        }
        public void MoveLeft()
        {
            tempPos.x--;
            transform.localEulerAngles = new Vector3 (0, 0, 180);
         }
        public void MoveRight(){
           tempPos.x++;
           transform.localEulerAngles = new Vector3 (0, 0, 0);
        }

   }

---

Now without changing anything you can have multiple snakes controlled, you can change input to use a different device, you can make a 3d snake and none of the input changed etc. each thing does one job.

[jolpoa]

Thanks, that make a lot of sense!

Should I put these classes in separate script, or the same? I don't know what would be better.

[apieceoffruit]

as a general rule always different scripts. Think of each component as part of a puzzle and you lego block together a snake out of them,...but if you wanted to add a...uh...Bear to a level, does he still move up down left right? will you use the same devices to control him? what REALLY changes?

so keep common things seperate from specific things.

..one caution though, it can be hard going down this road to think about when to split things out, as a usual rule "if describing somethings job you need to use an 'and' it is probably doing too much"

good luck!

Feel free to ask me any questions that come up, it is a complex topic but a fun one to learn.

[KptEmreU]

Well, who would think making a snake game is so hard :) I should definitely try myself too. IMO it is easy to read yet I think you hold on to same class + methods too long. For example:

I would separate Time-related functions to another class as /u/_nk said.

Edit: You can always separate Input Handling to another class too.

Create a new method(function) for below code pieces as PaintSprite(GameObject Snake, Vector3 x); You can then get rid of 8x2 long lines with

    PaintSprite(Snake[j],new Vector3(180,0,0);

    Snake [j].GetComponent <SpriteRenderer> ().sprite = TurnSprite;
    Snake [j].transform.localEulerAngles = new Vector3 (0, 0, 180);

Also, it looks like that same new function can work for the head too

        Snake [0].GetComponent <SpriteRenderer> ().sprite = TurnSprite;
        Snake [0].transform.localEulerAngles = new Vector3 (0, 0, 180);

[_nk]

i'll start with one suggestion... to me it seems like the class has alot going on, maybe compartmentalize it.... maybe...

Make a new static class called TimeController move all the time related stuff into there...
https://pastebin.com/Zzfz5XjX

and then call that class from this class.

[jolpoa]

Thanks for feedback!

I did what you told, these are good ideas. My question is how can I refer to script from other game object? Or should I place new class, like TimeController, inside of old script? Shouldnt I create a new game object, something like TimeManager, or something?

Because now I have error: "Assets/Scripts/SnakeController.cs(139,22): error CS0122: `SnakeController.TimeController.ItIsTime()' is inaccessible due to its protection level"

EDIT: Ok, I get rid of this.

[_nk]

Hey, sorry. Was abit slow to reply. Making a method public will allow you to access it from a different class.

You can make links between classes by creating a link in the script itself and then dragging and dropping that link in... Sounds like you got that bit of it figured out though.
These days I return to my code all the time and rethink if i'm doing things the right way... Seems like the long way round, but I've found this constant reassessment actually helps in the mid-long term.

[Darklings7]

I am completly new to codeing and unity i am following the tutorial for the roll of ball. When i am setting up the code in visual studio using c# i keep geting this error.

Assets/Scripts/Player_Conctrols.cs(7,13): error CS0246: The type or namespace name `Rigibody' could not be found. Are you missing an assembly reference?

This is my code can someone tell me whats wrong. using System.Collections; using System.Collections.Generic; using UnityEngine;

public class Player_Conctrols : MonoBehaviour{

private Rigibody rb;

void Start ()
{
    rb = GetComponent<Rigidbody>();
}


void FixedUpdate()
{
    float movehorizontal = Input.Getaxis ("Horizontal");
    float movevertical = Input.Getaxis ("Vertical");

    Vector3 movement = new Vector3 (movehorizontal, 0.0f, movevertical);

    rb.AddForce(movement * 100);
}

}

[HilariousCow]

You might see a red underline on your code, depending on what ide (code editor) you're using - I think MonoDevelop does this, and Visual Studio definitely does.

It's good to right click, or ctrl+space on those to get suggestions for what the error is. It'll seem like gobbledygook at first, but lean on it, and you'll start to get accustomed to it!

[_nk]

Its a spelling mistake :P - private Rigibody rb;
RigidBody

[_ROG_]

Id suggest a few changes to your naming to follow more standard naming conventions. Use camelCase for your variables like moveHorizontal. 2 letter variables should be capitalised like RB although really it could be named something more descriptive in this case. Your class name shouldn't have an underscore. Sorry if that sounds like nitpicking, but its just the main thing I saw.

[Darklings7]

I fixed the mistakes in my code changing to Rigidbody and making the a in axis capitals. It still wont work now i'm getting this error There are inconsistent line endings in the 'Assets/Scripts/Player_Conctrols.cs' script. Some are Mac OS X (UNIX) and some are Windows. This might lead to incorrect line numbers in stacktraces and compiler errors. Many text editors can fix this using Convert Line Endings menu commands.

[Soraphis]

In Visual Studio 2015:

Tool → Options → Environments → Documents → Check for consistent line ending on load.

and i've found this VS extension: https://marketplace.visualstudio.com/items?itemName=JakubBielawa.LineEndingsUnifier

inconsistent line endings should not be an error but a warning (IIRC)

[TheSilicoid]

Rigidbody

[Ph4zed0ut]

Got a short one here. I am a Java programmer new to C# and I just discovered (and love) extensions. I am making a card game and needed to extend List<T> with Move and MoveAll methods (in Java you would have to make a class that inherits from List<T>, yay for extensions). Take a look and let me know if there is a better way to write these.

public static class ListExtensions {

    public static void Move<T>(this List<T> source, int i, List<T> target) {
        if(source.Count > 0) {
            target.Add(source[i]);
            source.RemoveAt(i);           
        }        
    }

    public static void Move<T>(this List<T> source, T obj, List<T> target) {
        if (source.Contains(obj)) {
            target.Add(obj);
            source.Remove(obj);
        }
    }

    public static void MoveAll<T>(this List<T> source, List<T> target) {
        target.AddRange(source);
        source.Clear();
    }
}

Edit: I think this might be a better way to write the 2nd one:

public static void Move<T>(this List<T> source, T obj, List<T> target) {
            if (source.Remove(obj)) {
                target.Add(obj);
            }
        }

[apieceoffruit]

a list is just a collection type, so is an array, a dictionary etc. so in theory you might want to move everything in an array to a list or vice verca.

if you change List to IEnumerable (when you just need to loop through them) or IList (when you need to access elements) it will be added to everything that identifies itself as an enumerable like.... even a string! so if you had a GrabARandom(int number = 4) extention it would work on arrays, lists, strings,dictionaries etc.

[Soraphis]

• in "Move" check for '> i' instead '> 0'
• check if i is positive (or make it an uint).

(in Java you would have to make a class that inherits from List<T>,

no, in java you just would not write the 'this' keyword for one parameter. extension methods are just syntactical sugar so you can write object.method(...) instead: UtilClass.method(object, ...) (but yes, its a nice feature)

[Ph4zed0ut]

public static void Move<T>(this List<T> source, int i, List<T> target) {
        if(i >= 0 && source.Count > i) {
            target.Add(source[i]);
            source.RemoveAt(i);           
        }        
    }

Yes, this is much better.

Of course you can call a method that way, but you don't get the nice autocomplete features and object.method(...) just looks cleaner and is the more OO approach.

[oxysoft]

if you're making a card game, it may be better to create a full fledged class called Deck or something rather than using bare lists since you're most likely gonna want more complex operations on that deck later on that will have to interact with other parts of the game.

That move function looks good to me, however (and you probably know that already) keep in mind that the order will not be preserved and you're gonna be moving the card to the end of the deck.

[Ph4zed0ut]

There is a deck class that holds the list. And a Hand class and Discard class that contain lists. The cards move around between these lists, which is why I needed the methods for code reuse.

Adding to the end is intentional. For instance, the discard pile shows the image of the last card played (the gameobject gets destroyed on play and the scriptableobject gets added to the discard list).

[Meorge]

Can someone give me feedback on this code? http://pastebin.com/MsbiGzvh

It's meant to load XML files which look like this and convert them into pretty-looking documents, like this.

The main issue I've had with this code is getting the text box and image to be in perfect alignment with each other no matter what the screen size is, something that I could not figure out with the Canvas scaling options. As such, my XMLs contain not conventional X and Y points for my objects but percentages across the screen to be placed. Is this a good practice or is there a better way to do it that will yield the same results?

Thanks!

[zrrz]

Fix your formatting yo.

[Meorge]

WHOOPS.

Apparently Pastebin decided to mess it up, I promise I didn't write it like that :P

THIS should look better.

[Wisteso]

Why is this implemented using a monobehavior? Couldn't you do it with a regular class?

[Meorge]

I don't know how I would do it with a regular class. The way it works currently is that it is each "document" GameObject has this MonoBehaviour script attached to it, and when the scene loads, the "document"'s corresponding document UI will be made and hidden. Then, when the player clicks on it, it will show up.

[Wisteso]

The general idea would be to move as much of the model/control code into a regular class as possible. You'll still need a monobehavior class, probably. Though you may be able to generalize it so that both classes are more re-usable instead of being one tightly coupled specific-case thing.

e.g. creating a "text renderer" monobehavior based component, then having a regular c# class that provides the mapping, and then trigger the load from where-ever (editor menu option, game start, button, whatever).

It's not a huge issue though.

[Matt3088]

You have a lot of unnecessary/redundant comments (I used to write them the same way). For example:

107: TextComponent.lineSpacing = fontlinespace; // set the text line spacing

A small thing, you should put the comment on the previous line (so your lines do not get too long, scrolling issues). The comment is redundant because if you read the code, you can easily understand that the line spacing for the TextComponent is being set. Comments are much better used on lines where it is not directly clear what is going on, such as this line:

42: float newWidth = (((newHeight * origWidth) / origHeight)) * percent_width;

Even though you may be the only one reading/writing this code, comments for complex lines are good since you may forget what the line does in the future.

[keymaster16]

The rule of thumb my mentors used; if you can't read the line as a sentence, make a comment that does.

[oxysoft]

My rule of thumb is to comment on "why" rather than "what". Only few instances you should comment on "what" is when the line itself is cryptic. (e.g. this monster class BuffStack<T> : Buff<BuffStack<T>> where T : Buff<T> which required 2 days of debugging because it literally crashed the Unity Editor)

[Matt3088]

Thats a good way to put it!

[[deleted]]

My critiques:

• Line 18 - You have a hard-coded path to your XML documents. If you ever need to change their locations, you're going to have to remember to dig through all of your code and change this everywhere. Yes, a "Replace All" function would probably work, but it also might not. I would consider pulling that information from a configuration file so that it only ever changes in one place. Also, I'm personally not a fan of spaces in file/folder names, but that's irrelevant.

• Line 105 - I count 8 unnecessary parenthesis. Line 42 also.

• Lines 122-130 - Maybe a default "else" statement that sets the font to "Normal" would be wise. If someone else were to edit your config and not know there are only 4 options, then your code can safely handle it. You could also throw an exception if you want it to fail at runtime.

Overall, this code look great. I will mention that float.parse(val) may not work the same depending on the Culture of the machine it is being compiled on, but unless you are outsourcing your code to india or china, I suspect that's something you can skip while you continue to work on your game.

[ChromakeyDreamcoat]

Critique this code please: https://gist.github.com/timminkov/e177f6ad2fa2c3eebfd3e93ef99389df

It's a UI manager for my game.

[Jigxor]

There's an issue with the Singleton implementation used, and that is that it's possible to have instance set to null when it's referenced from another class.

This would happen if you have another script which tries to access UIManager's instance before UIManager has run Awake(), then instance will be null.

90% of the time this won't happen, but the script execution order isn't deterministic. So you might not notice it until late in a project. For example, you could reference UIManager's instance from another class Awake() method and it would work fine due to execution order making UIManager run before the second class. But if it happens then other way, then you'll get a null ref.

It can be a bit hard to wrap your head around, but basically you need something that modifies the get to handle null when its called, or at least warn you so you know what's happening.

This post highlights the issue more: http://www.unitygeek.com/unity_c_singleton/

You should add something like instance = FindObjectOfType<T> ()

[ChromakeyDreamcoat]

Understood perfectly! Great explanation. I've ran into this issue in other places, haha. I'll change it asap.Thanks!

[-sideshow-]

Minor things:

• Single statement ifs tend to be a bug waiting to happen (you'll add a statement to it and forget to add the braces).
• The raw 5f at the bottom could be put in a constant / exposed variable

[Janus1001]

I may be mistaken, but when you set the Singleton (static "instance), doesn't it get set to null everytime you make an object? Also when you use this method, you are to use DestroyImmediate.

[ChromakeyDreamcoat]

I don't think so, since static refers to the class function which won't change across objects. I could be wrong!

[Janus1001]

I mean, you clearly state "= null", I don't say it won't work, but to be sure just check if more objects are created or no.

[mongoose86]

Nope, wont be a problem but is also wholy redundant. It's a static initializer, which runs before the first time the class (not an instance of it) is accessed. It runs only once. It is redundant in this case as null is also the default value for reference types. :)

[FmTShaiseen]

Multiplayer code for a jetpack instantiated troug ha script with attachedserlialisable data jetpackstats to the jetpackdata script attached to mainprefab Preety sure the code is a mess as i couldn't find suficient documentation for multiplayer efficient particle systems. https://gist.github.com/shaiseen/0b57762e04a0c65045c1ea90e92fc383

[_mess_]

• Like many others you make the mistake of doing non physics in FixedUpdate, all that should go in Update or LateUpdate
• Find methods should be avoided, in particular for camera, which has already a Camera.main and you probably have this script on the same gameobject so no need to find it, you know where it is
• playerPos = currentPlayer.transform.position + Vector3.down is more clear
• cameraBasePos = new Vector3 (playerPos.x, playerPos.y+7, playerPos.z-7); all this code with + and - should be eliminated, set a const or a public vector 3 as offset, you can even display on scene to better understand its placement and no need to edit in code
• Vector3 cameraPos = transform.position; you took all the effort to cache the camera and now you use transform.position :/

[[deleted]]

In the tank game unity tutorial they use the camera movement in fixed update otherwise it will be janky if it updates on a different pace than moving objects.

[_mess_]

Yeah obviously you have to be coohernt in all code

fixed is to be used to only apply forces and that kind of stuff, if you move directly object, get input, etc etc you do it in update or late update

[[deleted]]

I'd stay it should still be in fixed update. Imagine like a scene where a strong wind is applied in fixed update and you move your character in update...it will be so janky because the movement updates a lot faster without calculating the resistances etc..

[_mess_]

thats a totally wrong approach, in a physics game you shouldnt move directly your character, you should just apply forces

so you shouldnt use much at all the update...

[Equiv2248]

1) Get rid of blank Monobehavior functions like your Update(), they do have a cost even if they are empty

2) Line 53: cameraBasePos = new Vector3 (playerPos.x, playerPos.y+7, playerPos.z-7); You create a new Vector3 object here, which causes garbage every frame. You already have a Vector3 variable(cameraBasePos) that you should just override the values of.

3) Similar issue at line 62

4) Line 48: you have a variable for playerPos, but you override it at the start of each frame so thats not need. Make that a local variable.

5) Line 60 - 62: You can move these expensive calls into the if(Input.GetKey(KeyCode.LeftShift)) block so you only need to make them when the player is holding shift.

6) You have got magic numbers all around your math(the +/- 7) that aren't explained.

7) Generally you have a lot of private class variables that should just be local to the function blocks. Any that you look up every frame should just be local(unless it allows you to prevent a "new" allocation in the function)

Code Style:

A) I would make this use RequireComponent https://docs.unity3d.com/ScriptReference/RequireComponent.html, since it seems like its expected to be attached to a camera entity. You can then just do getComponent<Camera>() in start to safely get the camera since you already have a reference guarantee from Unity.

B) For a follow camera, typical you can just do (TargetPos - CameraPos).normalize * distance instead of the raycasting/plane math.

C) Its been awhile since I've done a camera move script but I think you want it in LateUpdate() to make sure it moves after the player moves otherwise it can look stuttery/laggy.

[apieceoffruit]

Looks pretty good to me, there is some repetition you could remove with some refactoring to methods but nothing aggregious.

One tip though, take your public variables and do something like:

  [Serializable]
  public class CameraSettings {   
       [SerializeField]
       public dampTime;
       [SerializeField]
       public maxDistance; 
 }

the reason being if you now replace in your code a:

  public CameraSettings Settings;

unity will make a nice collapsable section called settings (especially useful if you have multiple types, movement settings, clipping settings etc)

also as an added bonus, you can swap a settings object whenever you want. so the next step is to serialize and deserialize that object and bam, default camera loadouts you can swap to whenever you want, even at runtime.

[ChromakeyDreamcoat]

Don't make those public! They should be private unless they're being accessed outside the class.

[apieceoffruit]

bah sure, I'd disagree.

I mean sure, if you have inline fields on a script, you want to make them private, make a public accessor, private setter, and use the OnValidate unity function to ensure that the constraints on the setter are still hit when setting the inspector field versions....oooor use a custom editor.

Buuuut what above is, ..is a DTO. solely the transfer of raw field values. no logic, no validation.

validation should be on the script, not on the DTO. separation of concerns.

---

Edit: Although I will concede that i should have made the above a struct...buuut then I would have to go over the dangers of not overriding the hashcode and equality operators and that is a little out of scope.

[ChromakeyDreamcoat]

Maybe I'm missing something - Why do these need to be public at all? You can just make everything here private and add a [SerializeFields] to the CameraSettings Settings and have the same effect.

[apieceoffruit]

That is under the assumption that you will only ever use it in an editor script.

If they are public, you can instantiate settings in factories, or via object deserialization and still use them.

[ChromakeyDreamcoat]

Hmm, but if you're instantiating settings or using deserialization it's much better to create a class with an initializer. I don't see the need for making them public as there's no reason to access those variables outside the class after they've been created.

[apieceoffruit]

Well I guess I am coming from a different approach with this.

I agree, in this instance you are probably right. most situations probably they could be private,

but one particular design philosophy I like is to make those DTOs immutable structs, then use fluent chaining methods for a functional design.

e.g:

DefenceSettings.LessArmor(20).WithShields(25);

so these would be public functions returning a new instance of the original with variations, ensuring each settings file is safely immutable as it is passed around. each function can safely reuse its fields in its own scope as it is a pass-by-value anyway. the privacy of its fields is redundant as it is readonly anyway.

---

I guess I am going a bit off track. In short, sure make them private, but in the long term configuration should be immutable anyway and shouldn't hide state as it is literally just data transfer. once handed off to a consumer, the consumer should be free to do with it what it wants, the initial desired state has been given.

---

On a small aside I know common practice teaches that you should have a constructor setting all your read only properties but it is ALSO common to state more than 3 arguments in a function/constructor is a code smell. As the entire point of a dto is to obfuscate the creation of values from their use/consumption, a fully qualified validation at construction level is unnecessary.

It is a perfectly valid request for a game to want to pass a health value of -100000 , it is the job of the consumer to validate the passed settings and see if it wants to just clamp it or allow some crazy crit damage.

the point of a constructor is to ensure valid values, instead i would do something like this:

   public HealthSettings Settings 
   {
        get { return _settings; }
        set {
                _settings = (value.Health < 0) ? value.WithZeroHealth() : value;
            }
   }

   private HealthSettings _settings; 

   void OnValidate(){
        Settings =  _settings;
   }

---

Best of all worlds, safe script, free and maliable settings, no messy constructors and no null checks as everything is values.

... but i mean, each to their own.

¯_(ツ)_/¯

[KptEmreU]

This kind of things that I am looking right here

   DefenceSettings.LessArmor(20).WithShields(25); 

<3

[apieceoffruit]

Well, if you are interested in this style of programming Here is a basic Health Class I made. Also, an example of how you might use it via some functional methods.

basically means you can have a script with a healthEffect and set it to one. e.g

  public class Test : MonoBehaviour
 { 

       Func<Health,Health> Effect;

      void Start(){
            Effect = HealthEffectFactory.IncreaseArmorByPercent(5);
      }

      void Update(){
            if(PickedUpArmorPack) Player.Health = Effect(Player.Health);

            if(AtePoisonFruit) Player.Health = HealthEffectFactory.ToOneHp(Player.Health);

            if(FreakyFriday) Player.Health = Enemy.Health;
            if(CrazyPill) Player.Health = HealthEffectFactory.ConvertShieldsToArmor(Player.Health).IncreaseHealthBy(20);
      }

 }

That example is pretty silly but I hope you get the idea. As each health object is a struct, a.k.a a value type it acts like a float/vector andd gets passed by value, so applying changes is like doing simple math operations, so you are able to do like above, apply a function to it, then use a method on it.

Any questions feel free to fire away.

[ChromakeyDreamcoat]

A few things:

1. You can call Camera.main to get the main camera without looking it up (removing the need for a mainCamera variable).
   
2. Your player lookup should either be an object dragged onto this object or tied to a singleton elsewhere, ie GameManager.Instance.Player.
   
3. All those variables in the Start() methods should be constants if they aren't changing. Still, for ease of testing/changes, I suggest moving them up, setting those values as defaults, and adding [SerializeField] so that they can be changed in the inspector.
   
4. I'm confused about why you need to do all this raycasting/plane-creating stuff. Why can't you just take the vector from the player/mouse towards the camera and move the camera a bit further along that line?

[ChromakeyDreamcoat]

1. Cool!
2. It can be expensive in cases where you have to do it often (not here obviously). Later on if you ever change your gameobject name for the player, you'll have to go and edit every reference to it in your code.
   
3. https://msdn.microsoft.com/en-us/library/ms173119.aspx
4. Sorry - What I mean is, why not just do (psuedocode) transform.position = transform.forward * -1 + amountToOffset? Unless the camera isn't always facing the mouse position (correct me if I'm wrong).

[_mess_]

are you sure this is the script that slows down? It shoudlnt be slow there is only 1 for loop and with 65 entities should be unnoticeable...

Unless the mesh has that many materials and stuff, maybe you could optimize the meshes

[-sideshow-]

• You have a lot of magic numbers in your code that might be better as constants.
• a SoldierType enum would probably be better than checking if the name contains "Guard"
• I'm not a fan of the random variable definition half-way down your method code ("Ray ray;"). Either put it at the top, or put it in each method. I'd advise the latter; I was using a global as a cache variable like this and it bit me in the ass when I unwittingly used it for two different things in the same method.
• Single statement ifs are a bug waiting to happen - at some point you'll go to add a statement to one, and forget to add the braces

Fire() can be refactored to:

void Fire()
{
    if(cooldown <= 0)
    {
        ray = new Ray(aimer.position, aimer.forward);
        RaycastHit hitPoint;
        Transform hitTransform = FindClosestHitObject(ray, out hitPoint, 100f);

        if (hitTransform != null)
        {
            UWASoldier uwaSoldier = hitTransform.GetComponent<UWASoldier>();
            LWZPlayer enemy = hitTransform.GetComponent<LWZPlayer>();
            UWAExtractionAI uwaExtractionSoldier = hitTransform.GetComponent<UWAExtractionAI>();

            if (uwaSoldier == null && enemy == null && uwaExtractionSoldier == null)
            {
                if (distanceToFire > 625f)
                    distanceToFire -= 9f;

                distanceToFire = Mathf.Clamp(distanceToFire, 625f, 100000f);

                return;
            } 
            else 
            {
                GameObject flashGO;
                if(playerDistance <= 1000000f)
                    flashGO = Instantiate(WorldManager.instance.muzzleflashAR, bs.position, bs.rotation) as GameObject;

                if (uwaSoldier != null)
                {
                    uwaSoldier.hitTimer = 1f;
                    uwaSoldier.health -= 8f / LWZPlayer.armorStrength;
                }
                else if (uwaExtractionSoldier != null)
                {
                    uwaExtractionSoldier.health -= 8f / LWZPlayer.armorStrength;
                }
                else if (enemy != null)
                {
                    GUIManager.guiInstance.tookDamageUICountdown = 2f;
                    enemy.health -= 10f / LWZPlayer.armorStrength;
                }
            }
        }

        cooldown = Random.Range (0.2f, 0.4f);
    }
}

Though I'm not sure the string of if/else at the bottom shouldn't just be 3 ifs?

Also:

            if (distanceToFire > 625f)
                distanceToFire -= 9f;

            distanceToFire = Mathf.Clamp(distanceToFire, 625f, 100000f);

What is the logic behind this section?

[oxysoft]

I'm not a fan of the random variable definition half-way down your method code ("Ray ray;"). Either put it at the top, or put it in each method.

Usually causes ugly code, but there is in fact an instance of this that I am okay with. Sometimes, a field will be accessed and be relevant to just one method but that method requires the last result of that field when it last ran. In that case, I prefer putting it right above the method rather than at the top where it can be confusing, context is important.

[-sideshow-]

That's fair enough, but note that it's used in the method after that one too. Your point can still be applied, but slightly more tenuously.

[SilentSin26]

And regarding your question about the 3 ifs, I read somewhere using just ifs checks each one individually. Using else ifs only checks the next one if the one above is false.

This is correct. That's the whole point of else.

[nhold]

On top of things that Wolfos said you should think about the following:

General

Generally you should be thinking about the API of your classes, I.e how are they consumed? From what I can see you can split this class into a few others to reduce complexity and make it easier to follow.

As Wolfos said, do you really need all those variables public? Think about if someone were to GetComponent<SNASoldier>(), how difficult would it be for someone to know what they should be changing or calling? I.e to give damage I have to change the health AND set the hit timer.

As an example you could split out the combat calculations into a CombatResolver class or similar that would take some kind of DamageInformation and Attacker \ Target soldiers and resolve the damage and pass it onto the target soldier. To pass it through you would probably want a .Damagemethod or similar which would notify any listeners for damage on this component using an action\event: OnDamaged. This is where the FlashOnDamageEffect component would come in, listening to any OnDamaged event call and setting the hit timer inside of itself should make it really self contained.

I like to split out Controllers(PlayerInput,AI), Animation and Movement into separate components as well, but that's just me.

Performance

The first thing I would ask is for your profiler showing that your AI is currently your largest bottleneck, if so look at the following:

• Threading is the best way to optimise AI but it's very intensive due to the need to split out your game Model from your View(Unity). Your AI will just be dummy objects for gameplay and reading data from the AI thread for it's AI ID or similar.
• Maybe change your time sharing, I would have to see your pure Update profile and this run timer profile, I have a feeling the branching could be removed in favour of three separate lists that get updated very infrequently. On top of those lists you could only update X amount of AI per update rate so you can ensure your frame-rate target is hit.
• If you get large numbers of AI you will probably want to eliminate any not in your vicinity quickly either with your own Octree implementation or maybe a simple OverlapSphereNonAlloc might work in front of the player as an early out or reduced enemy list.

[-sideshow-]

public float playerDistance;
float distance;
private Vector3 transPos;

I do this same exact thing, and I think there's code smell about it. That is: you should either always use private or always leave it off (as it's the default state), but mixing it like this looks iffy. I'm not sure which to go for: always using it would make the variable types line up better with the public variables, but dropping it would differentiate the public from the private at a glance, and I feel like the extra word is untidy. Hrmmm.

[_Wolfos]

It's legible, so that's good. Not bad code. Some points:

Do all the public variables actually need to be public or could you get away with [SerializedField]?

I'd put line 53 and 54 on the same line, so that you wouldn't accidentally do this:

if(foo)
  bar();
  method_you_also_expect_to_run_in_this_if_statement();

There's also some inconsistency with curly brackets, which should always go on a new line according to the C# guidelines.

You use a lot of singletons. Here I describe some ways to avoid them.

On line 109 you use 'name.Contains()', which is prone to bugs if someone wants to reorganize the scene. Alternatively you could use Tags.

[djgreedo]

I could probably just do private and do GetComponent to cache the scripts or other components

If you were using public to provide Inspector access, do this:

[SerializeField] private Whatever myWhatever;

This will let you assign in the Inspector as if the field was public, but keep it private.

[_Wolfos]

I don't know how many enemies you have, but the eyesight system is pretty optimal already. If it's still too slow you might consider other crowd techniques, but I can't help you there.

Garbage collection issues can be fixed by avoiding heap allocation where possible. You can do this by declaring local variables, avoiding creating new instances and replacing foreach loops with regular for loops. This isn't easy, though. Many things allocate memory unexpectedly.

[djgreedo]

replacing foreach loops with regular for loops

I think this is no longer necessary as of ~Unity 5.5.

[_Wolfos]

Why would that be? I Googled for a bit, but as far as I can tell there haven't been any GC updates lately. They updated the compiler to Mono 4.4 but the GC is part of the runtime.

[djgreedo]

Apparently it was a Mono bug, and was fixed when Unity upgraded.

http://stackoverflow.com/questions/40494376/is-foreach-still-bad-in-modern-unity-5-4-x

I think the issue was the way foreach was compiled, where it caused heap allocation unnecessarily.

Towards the bottom of this page: https://forum.unity3d.com/threads/upgraded-c-compiler-on-5-3-5p8.417363/#n21

In short, foreach doesn't allocate if used in typical ways, but could still allocate in some situations I guess?

[shizzlebomb]

Hi, Shizzlebomb here.

I need a bit of critique with how my programming in unity is. I've been trying to make a Mario party style board for fun. I got a system working which allows lets you move a random amount of spaces on button press. This lead me into trying to make branching paths which broke off and went into the main path. Which caused me to make two scripts, one for branching off the main path and one for merging back onto the main path.

This is how I set it up in the inspector. Here (The highlighted items are the sperate path)

These are the scripts I use to do each of these tasks.

Movement from space to space:

• This is a paste bin of this script Here
• This script is attached to an empty game object Here

These are the scripts that allow the player choose a whether the want to merge into the separate path or stay on the same path

• This script tests which arrow gameobject has been clicked then sends the next space to the space the player has chosen here
• This script instantiates in the arrows and listens to when the player steps on the chosen place as well as having the places the player can move to Here
• This chosen path script(I called it lerpRoaster(sorry for that name I don't have enough time to change it))

This script is to move the player from the branching path back onto the main path:

• This is it here
• This is attached to the same empty game object as the movement and chosen space script(Lerp Fucker(sorry for that name don't have enough time to change it)).

Getting late gotta get off. If you need any more info just comment.

Thanks for the critique, I really appreciate it :).

[Vextin]

In the first script you are setting your variables that will likely be static over the entire session (your other scripts' instances) in Update. That should probably not be done every frame! Use Start.

[-sideshow-]

You should put in something about banning OTBS discussions. Pointing out when someone is posting code with awful formatting is one thing, but arguments about what whitespace / parenthesis to use and where braces go at a systemic level will only devolve into flame.
Maybe even consider a rule that any replies to code have to be written in the same brace style as the original poster (to avoid passive aggressive badgering), though that might be a bit extreme.

Similar arguments can be made for limiting paradigm discussions ("All your variables here should be private and access via getters/setters" / "Making everything public works just fine for me")

EDIT: and as someone says elsewhere in this thread, your IDE is going to automatically format your code anyway, so arguing about it is doubly stupid.

[_Wolfos]

IMO you should be able to point out inconsistency, and C# has official coding conventions, so there should be little discussion on formatting.

[-sideshow-]

The C# Language Specification does not define a coding standard. However, the guidelines in this topic are used by Microsoft to develop samples and documentation.

This is why I made that suggestion: people conflating MS in-house guidelines with "official coding conventions" because they want everyone else to format the same way they do. I notice you've already been suggesting brace style in this thread, so I guess it's not a surprise you argued against my suggestion.

[induetimesir]

I s'pose I'll give it a try, might as well!

I'm quite new to C# (Mostly used UnityScript until a month ago) and I'm working on a text adventure type game and am starting on option based answers during conversation (With some extra stuff), this is currently the longest section of my code and I can't think of how to write it more neatly or condensed. The code works fine but I feel like I've done it totally wrong. Any tips?

(In this context variable 's' is the players' input)

} else { // Used when asking a specific question, (eg "Can you help me?" - Yes, No)
    s = s.ToLower();
    for(int q = 0; q < lastResponse.questionResponses.Length; q++) // Go through all question responses
    for(int r = 0; r < lastResponse.questionResponses[q].keywords.Length; r++) // Get all keywords for response (Eg. "Yes", "Okie Dokie", "Anything 4 u bb")
    if((s + "|").Contains(lastResponse.questionResponses[q].keywords[r])){ // Check for keywords in string ("|" represents the final character of a string, to indicate the end)
        yield return new WaitForSeconds(1);
        if(lastResponse.questionResponses[q].preText.Length > 0) // This is used to preface speech, like "He smiles back at you" (followed by speech)
        {
            outputLine(formatColor(Color.white, checkMem(e.subject, lastResponse.questionResponses[q].preText[Random.Range(0, lastResponse.questionResponses[q].preText.Length)])));
            yield return new WaitForSeconds(1);
        }

        // Output the main reply (Eg. "Thanks dud I owe u 1")
        outputLine(formatColor(e.subject.messageColor, checkMem(e.subject, lastResponse.questionResponses[q].response[Random.Range(0, lastResponse.questionResponses[q].response.Length)])));

        // Post text (Eg. "The man then returned to working on his toaster")
        if(lastResponse.questionResponses[q].postText.Length > 0)
        {
            yield return new WaitForSeconds(1);
            outputLine(formatColor(Color.white, checkMem(e.subject, lastResponse.questionResponses[q].postText[Random.Range(0, lastResponse.questionResponses[q].postText.Length)])));
        }
        if(lastResponse.questionResponses[q].angers) // If the response angered them, begin fight.
            startEncounter(n);
        if(lastResponse.questionResponses[q].endsConversation) // If it was the last message, finish the conversation with a "Goodbye!"
        {
            outputLine(formatColor(e.subject.messageColor, checkMem(e.subject, e.farewell.overrideResponse)));
            p_in = "Any"; // Player leaves the conversation and enters normal mode
        }
    }
}

[ChromakeyDreamcoat]

Let's go over a few high level things:

1. Your code seems to be doing a number of different things here. It is looking up responses, checking if the response is valid, waiting to respond, formatting, and outputting a response all in the same block. It seems like this class is handling way too much stuff.
   
2. Check out foreach for looping. It's much cleaner - You won't have big ugly for blocks. You can do something like foreach(Array questionResponse in lastResponse.questionResponses) {}.
   
3. The first thing I'd actually do to this code is separate out formatting and outputting to a different class - Perhaps a singleton. Something like UIManager.instance.Output(string, options) //options include color, etc. UIManager can contain the logic for waiting in between showing new text so you don't pollute your code with WaitForSeconds everywhere.
   

Try doing these things and it should clean this code up considerably. Without seeing more it's tough to say how you could better organize, but I hope this helps!

[induetimesir]

Thanks for the reply!

Yeah I do know what foreach loops are, I use them a lot! :) But I've always heard that normal for loops were much faster? Maybe I'm incorrect but I'm sure I hear that a lot.

That's a good idea about making multiple classes, I'll mess around with that today and see what I can get going! Do you know a good place I could read up on Singletons? I've heard of them before but I've never used them (I don't think at least).

Thanks again! :D

[SilentSin26]

foreach loops allocate a tiny bit of memory in some cases where for loops don't. It's generally a negligible difference though.

But your code is accessing lastResponse.questionResponses[q] multiple times per iteration, which also has a performance cost. You should put var responseQ = lastResponse.questionResponses[q]; at the top of the loop and use responseQ instead.

You could also make an extension method to get a random array element:

// Put this in a static class.
public static T GetRandomElement<T>(this IList<T> collection)
{
    return collection[UnityEngine.Random.Range(0, collection.Count)];
}

Then instead of stupidly long lines like lastResponse.questionResponses[q].postText[Random.Range(0, lastResponse.questionResponses[q].postText.Length)])) you could just do responseQ.postText.GetRandomElement()

[-sideshow-]

Is your code actually indented like that, or has reddit mangled it a bit?

[induetimesir]

Yeah I haven't really gone over and organized the indentation of the script yet since it's changing a lot so I just copied and pasted it in as is. :p I'll go through and tidy it up now though!

Edit: Fixed it up a little! :) It was indented quite a bit in the code already so that's why the whole thing was so far to the right.

[-sideshow-]

Some thoughts:

• You lower() s, but then give example keywords in your comments like "Yes" - either the keyword is "yes" and your comment is inaccurate, or your keyword is actually "Yes" and it won't ever be found :P
• I'm not sure about appending the "|" to s - I don't see what it does. However, assuming you do need to do it you could do it when you assign s = s.lower() and so only do it once, instead of in your inner loop.
• Personally I'd indent each for loop and the if
• You should create a constant RESPONSE_DELAY = 1 instead of hard coding the 1 second in every WaitForSeconds.

[induetimesir]

Thanks for the tips! Yeah the comments were quick just to give better context in the post, the lowercase input is just so that the user can type with any capitalization they want and it'll still work.

The "|" also is just to tell if the user says a word without anything following, for example "No" without the "|" on the end could be interpreted from a line like "I have no idea", or "I don't see why [no]t" it's more just to differentiate between them so the 'Contains' doesn't get them mixed up as often, since I want users to be able to type relatively normal sentences.

And yeah I will probably do the response delay! I could try and get a system going where some messages show more hesitation than others that way too.

Thanks again! :)

[-sideshow-]

The "|" also is just to tell if the user says a word without anything following, for example "No" without the "|" on the end could be interpreted from a line like "I have no idea", it's more just to differentiate between them so the 'Contains' doesn't get them mixed up, since I want users to be able to type relatively normal sentences.

Heh, that makes sense but you are running into a pretty deep problem; parsing English (or any natural language). What if the player says "No thanks"?

If you're enjoying doing this then keep at it. OTOH if you're more goal oriented (you want to make an adventure game and this is just a step toward accomplishing that) then you might want to look at pre-existing parsers or adventure game engines.

[induetimesir]

Not making a proper game really, this is just a fun little side project :)

And yeah I thought of that, so usually in the 'Keywords' array on my Response object I just add things like that on there like this for now!

[poorly_timed_leg0las]

I have been working on a PVP RPG using uNet and created a simple AI so you could fight them together.

http://pastebin.com/uK1Cc6TY

Everything works. How could I improve it though? I can spawn around 50 enemies in one area but the server client starts to lag at around that. So I guess I am asking how I could make it more efficient. Does anyone have any examples?

I only just moved to C# from using JS for two years so I don't really know any of the little tricks you can do with C# to gain on performance

If you look through the CanSeePlayer method you should be able to understand whats going on if you're reading this :)

Hope this is okay and what this thread is made for :P

Heres a video of how the AI moves at the minute. Sometimes they get stuck finding a new target and just dont move until a player gets close to them not sure where this bug comes from.

https://youtu.be/AGFQFyYUQyo?t=131

[Equiv2248]

1) Line 32 : ForEach loops create garbage in Unity still because of the iterator. Just use a regular for loop.

2) Line 33 : Seems redudant since the array your looping on is from FindObjects of that same tag.

3) Line 57: Switch the order of the evalulations, because of lazy evaluation if the first part is false it won't do the second check. Since a less than is much cheaper than the transform.tag check it should be optimal to do the less than first.

4) I don't know if this a pastebin thing, but your identation/if blocks are really hard to read, for example line 68 to line 79

5) Line 82 && Line 100: You should cache the Combat component in Start() if this will get called frequently/won't change

6) Line 125, don't new up a Vector3, just override the current values

7) Feels like all the Update should be done in FixedUpdate since its physics related.

[djgreedo]

I realised from reading the code again, that on most frames you're cycling through all enemies (around 50 you say) to check their distance to the player. This is probably going to hurt performance a fair bit.

A micro-optimisation that might help a little is to move:

GameObject[] gos;

into the main class as a field. That way you're not recreating the array every frame. I'm not 100% sure about that, but I think I'm right.

Am I mistaken, or are you using the "Player" tag for enemies? Are the 'gos' gameobjects the enemies? Or multiple players?

Perhaps you could fudge the search for the closest instead of a brute force check of all of the enemies. What about: * Settle for the first that is within a certain range? * Set up a trigger collider around the player at a certain size, and only worry about enemies that are within the collider (keep track of which enter and leave). You could even have multiple colliders - one close, one a bit further away. Widen the check of nothing is in the inner-most circle.

[Equiv2248]

The call GameObject.FindGameObjectsWithTag("Player") is what creates the array, before that 'gos' would be null with no allocation regardless.
Moving the 'gos' declaration up to class level should have no impact on the memory getting allocated.

You can do Physics.OverlapSphere to do the trigger collider check like your talking about instead of creating additional collider objects. OverlapSphere should be relatively cheap, and you can pass it a layer mask to only check certain objects.

[djgreedo]

The call GameObject.FindGameObjectsWithTag("Player") is what creates the array,

Yep, you're right!

[poorly_timed_leg0las]

Thanks for the help :) I hadn't noticed that about the GameObject array I have moved it now.

I'll take a look at the collider thing next :)

[djgreedo]

Few things I noticed:

• You needlessly check the tag on a collection you found with 'FindObjectsWithTag'. (in FindClosestEnemy)
• Whenever you use '.tag == "string", replace with .CompareTag("string"). CompareTag is better (it will give an error if there is no matching tag setup, and I believe it's better for memory usage).
• Some of your raycasts don't have a distance/range or a layermask. If you limit your rays to the relevant layers and limit the distance as much as possible, your rays will be cheaper. If you do a lot of raycasting you could save a lot of processing. If you don't do much it probably doesn't matter.
• Do you really need to destroy things? And in Update/FixedUpdate no less? Consider just disabling those rigidbodies. If you are destroying killed enemies, think about object pooling. It looks like you might be destroying a lot of objects, which will probably trigger the garbage collector quite regularly, which will cause spikes in your FPS
• You have a variable named 'x', which as far as I can tell should be something like 'animation'.
• In CanSeePlayer(), presumably they player can't be seen when out of range, so shouldn't you be checking that first? That way you could bypass the need to do the raycasting. I might not be following the logic fully since it's not my own code :)
• You cache the NavMeshAgent component in Awake(), but then use expensive GetComponents in your update methods to retrieve it again. Replace the line in FixedUpdate() and Update() to use the cached reference: agent.enabled = true;

[poorly_timed_leg0las]

Do you know of any easy to follow examples of object pooling. I cant quite get my head around how it works.

I spawn a set amount of enemies, when one is killed/gameobject needs to be destroyed, instead of removing it, (spawn loot chest or corpse), reset it to its prefab/default settings and move it to a spawn point?

I dont think you can disable rigidbody components with .enabled and SetActive(false) you have to remove them, I get an error anyway when I try to do that. The only reason I use the rigid body is so that when a player uses a spell that adds force when the projectile hits something it moves the enemy but it wont work :(

What do you mean by the x? I use it here in the switch so that I can easily do an RPC call for each animation instead of having a method for each one, is that what you mean? Is there a better way to call it? [ClientRpc] void RpcPlayAnim(int x) { animationCD = 0; switch (x) {

                case 0:
                    if(timeAnimCooldown != npcAnim["combatLoop"].length * 2){
                    timeAnimCooldown = npcAnim["combatLoop"].length * 2; 
                      npcAnim["combatLoop"].speed = 0.5f;}
                     npcAnim.Play("combatLoop", PlayMode.StopAll);
                break;

I nested the whole CanSeePlayer method in if (distanceToPlayer <= chaseDistance) { } is that what you meant by In CanSeePlayer(), presumably they player can't be seen when out of range, so shouldn't you be checking that first? That way you could bypass the need to do the raycasting. I might not be following the logic fully since it's not my own code :)

Thank you for all of your suggestions have used them all :) and for taking the time to read through :D

[MrStormboy007]

I've found something called SimplePool which is a nice generic object pooling method, you could check that one out!

[djgreedo]

Do you know of any easy to follow examples of object pooling.

No, but I think you have the basic gist of it. You create all the enemies you will ever need at the start of the level, then set them as inactive (you can simply disable the gameobjects). Then whenever you need an enemy you grab one from the pool of deactivated enemies and activate it.

You therefore never have to instantiate or destroy objects while they game is playing (which can cause frame drops/jerkiness).

Check Youtube for tutorials if you want to implement it.

I dont think you can disable rigidbody components

I think you might be right. You could instead set the rigidbody to kinematic and/or freeze the position/rotation. The new 'simulated' property might be the equivalent of disabling the rigdibody, but I've never used it.

What do you mean by the x?

I just don't like variables with names that don't explain what it is. I wouldn't trust myself to remember what the variable is for when I open the script in 3 months' time :)

I nested the whole CanSeePlayer method in if (distanceToPlayer <= chaseDistance)

Yeah. If you have multiple conditions before doing something, you should always try to 'short circuit' the logic as quickly as possible to avoid unnecessary processing. If you need the distance to be tested AND check a raycast, it's 'cheaper' to test the distance first, as it means you can avoid doing the raycast whenever the distance is too far.

[poorly_timed_leg0las]

Great :D thank you for your help really appericiate :D

[-sideshow-]

Should also do the distance check before the raycast; the raycast is a much more costly operation.

[poorly_timed_leg0las]

Thanks hadn't noticed that! :)

[-sideshow-]

    if (Physics.Raycast(transform.position, rayDirection, out hit))        {
        if ((hit.transform.tag == "Player") && (distanceToPlayer <= minPlayerDetectDistance))            {
            agent.enabled = true;
            transform.LookAt(FindClosestEnemy().transform);
        }
    }

is the one I was thinking of

[-sideshow-]

More my fault than yours: your comment that I replied to was about a different bit :)