Managing and Referencing Attached Scripts Best Practice?

Hi Hoping somebody can answer whether I am heading in the right direction with my scripting layouts. using half pseudocode here. just makeing sure im not making something too simple or too complex. Below is a rough sample of 2 scripts i am using to handle the selection of a unit. only 1 unit can be selected at one time. is what im doing correct by splitting some info on the unitactions script which is attached to the actual unit or should i be making a mega script in player controller to handle everything? I dont want to make things more complicated for the game system but over seperating code or clumping too much code in one place.

I have a script attached to an overhead camera called PlayerController

public class PlayerController

Selected Unit objects
private GameObject selectedunit;
private UnitAttributes unitAttributes;
private UnitActions unitActions;    

//Variables
private RaycastHit mouseRayhit;
private readonly int groundLayerMask = 1 << 9; //Ground Layermask
Grid.GridCell mouseGridCell;

private void Update ()
{
if (Input.GetKeyDown (KeyCode.Escape)) {
Clearselectedunit ();
}
Physics.Raycast (cam.ScreenPointToRay (Input.mousePosition), out mouseRayhit, 100f, groundLayerMask);
mouseGridCell = Grid.GetGridcell (mouseRayhit); // Get a Grid Cell based on MousePosition raycast

	if (Input.GetMouseButtonDown (0)) { // When Left Mouse Clicked

		if (mouseGridCell.containsPlayer) {
			SelectUnit (mouseGridCell.containsPlayer);
		}
		if (selectedunit && unitActions.moving == false && mouseGridCell.walkable == true) {
			unitActions.PerformMove (mouseGridCell.position);
		}
	}
	// If the object is moveable and isnt already moving
	if (selectedunit && unitActions.moving == false) {
		ShowMouseMarker (); 
	} else {
		if ((mouseMarker))
			mouseMarker.active = false;
	}
}

	private void clearselectedunit ()
	{
		Debug.Log ("clearing");
		if (selectedunit != null)
			unitActions.selected = false;
		selectedunit = null;
	}
	public void ShowMouseMarker ()
{
	float height = mouseGridCell.terrainHeight + (unitAttributes.controllerHeight / 2);

	if (mouseGridCell.walkable) {
		mouseMarker.active = true;
		mouseMarker.transform.position = new Vector3 (mouseGridCell.position.x, height, mouseGridCell.position.z);
		mouseMarker.renderer.material.color = Color.green;
		unitActions.ShowMovePath (new Vector3 (mouseGridCell.position.x, mouseGridCell.terrainHeight, mouseGridCell.position.z));
	} else {

		if (mouseGridCell.containsPlayer) {
			mouseMarker.active = true;
			mouseMarker.transform.position = mouseGridCell.position;
			mouseMarker.renderer.material.color = Color.yellow;
		}
		if (mouseGridCell.containsEnemy) {
			mouseMarker.transform.position = mouseGridCell.position;
			mouseMarker.renderer.material.color = Color.red;
			mouseMarker.active = true;
		}
		unitActions.path = null;
	}
}

private void SelectUnit (GameObject unitClickedon)
{
	if (unitClickedon != selectedunit) {
		Clearselectedunit ();
		selectedunit = unitClickedon;
		unitAttributes = selectedunit.GetComponent<UnitAttributes> ();
		unitActions = selectedunit.GetComponent<UnitActions> ();
		unitActions.ChangeUnitSelection (true);
		selectedunit = unitClickedon;
	} else {
	}
}
    private void Clearselectedunit ()
{
	Debug.Log ("clearing");
	if (selectedunit != null)
		unitActions.ChangeUnitSelection (false);
	selectedunit = null;
}

public class UnitActions

	public bool moving = false;
	public bool selected = false;
	public bool performMove = false;
    private UnitAttributes uAttributes;

public void Update ()
{
	if (path != null && currentWaypoint >= path.vectorPath.Length) {  // Reached the end of the move
		moving = false;
		performMove = false;
	}
	if (performMove == true && uAttributes.moveable == true && path != null) {
		Vector3 dir = (path.vectorPath [currentWaypoint] - transform.position).normalized;
		dir *= uAttributes.baseMovementSpeed * Time.deltaTime;
		controller.SimpleMove (dir);
		if (Vector3.Distance (transform.position, path.vectorPath [currentWaypoint]) < nextWaypointDistance) {
			currentWaypoint++;
			return;
		}
	}
}
public void ChangeUnitSelection (bool toggle)
{
	if (toggle)
		renderer.material.color = Color.green;
	if (toggle == false) {
		renderer.material = defaultmaterial;
	}
}

    public void PerformMove (Vector3 targetPosition)
{
	performMove = true;
	moving = true;
	// Move to 'targetPosition'
}

public class Grid

public static float gridSpacing = 1f;

public class GridCell
{
	public Vector3 position;
	public bool walkable = true; // Assume cell is always walkable until Raycast hits collider
	public float percentCover;
	public GameObject containsPlayer;
	public GameObject containsEnemy;
	public float terrainHeight;
}

public static Vector3 GetGridPosition (Vector3 convertPosition)
{
	
	float newx = Mathf.Round (convertPosition.x / gridSpacing) * gridSpacing;
	float newz = Mathf.Round (convertPosition.z / gridSpacing) * gridSpacing;
	convertPosition = new Vector3 (newx, (convertPosition.y), newz);
	return convertPosition;
}

public static GridCell GetGridcell (RaycastHit mouseRayhit)// Vector3 position)
{
	GridCell cell = new GridCell ();
	cell.terrainHeight = mouseRayhit.point.y;
	Collider[] colliders = GetGridContents (mouseRayhit.point);
	foreach (Collider col in colliders) {
		//	Debug.Log (col);						
		if (col.tag == "PlayerUnit") {
			cell.containsPlayer = col.gameObject;	
			cell.walkable = false;
		}	
		if (col.tag == "EnemyUnit") {
			cell.containsEnemy = col.gameObject;
			cell.walkable = false;
		}
		cell.position = GetGridPosition (mouseRayhit.point);
	}
	return cell;
}

public static Collider[] GetGridContents (Vector3 checkPosition)
{
	return	Physics.OverlapSphere (checkPosition, gridSpacing);
}

I think it would be better to do this in a more clearly functional fashion. Instead of setting a ‘performMove’ tag to ‘true’, you should simply call a function that performs the move. Not only is it clearer from a logical perspective, it’s also cleaner in terms of the update loop.

So, where you have all that ‘performMove’ stuff, replace it with this:

unitActions.PerformMove(targetPosition)

Then in your UnitActions class have a function that goes like this:

public void PerformMove(Vector3 targetPosition)
{
    // Move to 'targetPosition'
}

Edit: I merged my three posts

about selectedunit, unitClickedon and clearselectedunit

First of all, check out your favorite coding convention and then stick with it every line of code. Best of all would be to try the coding convention from unity devs. UpperCamelCase for functions and lowerCamelCase for variables.

Code with variables like myawesomevariable is unreadable and unmaintainable as soon as it gets more complex. Better is myAwesomeVariable.

While coding, you spend more time reading and reviewing your code than actually writing. So spend the time in formatting your code strictly - it will pay 10 times off in less errors and more relaxed coding and bughunting!

so => selectedUnit, unitClickedOn and ClearSelectedUnit()


Why raycasting in this case if the mouse is not clicked? If you do costly operations like this and then ignoring the result as a standard procedure in many objects, your performance will drop without any win.

Better put it under the if condition where you really need it.

 if (Input.GetMouseButtonDown (0)) {
    // Only in this case you need the raycast

Ok three more things,

why setting the renderer color every update (30 - 400 or more times a second) if it’s state only changes while selecting or deselecting? This is ok for 1 or 10 units. But 100? 1000? Do not train such style. Doing games means performance fist with every thought!

Second thing iss the movement. You unit will never arrive, it will vibrate around the waypoint as long as performMove ist true. And again here, why calculating the direction every frame, if it only changes when the user clicks on ground?

Third is, why is only the line moving = true; inside the if condition? The rest will be calculated and called and whatever every frame, for every unit you have. This loss surely drops framerates already.

Allways keep the perfromance in mind. Making it run is one thing, but making it run fast is another. And fast you want the AI, Pathfinding, UI etc. etc. to have more power left for the graphics and explosions! :wink: