koobas.hobune.stream
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'
}
The reason i did that was the move code is actually dependendant on update as it is using a charactercontroller as below code. so is that correct to leave it in the update?
There's no reason why 'Perform$$anonymous$$ove' doesn't just change a 'target position'- you can leave the actual moving inside of Update, but it's a cleaner program if sending commands to an object occurs in functions (since that's kind of the point of them).
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! ;)
Thanks Gone ahead and cleaned up my na$$anonymous$$g conventions. I usually do try and follow a standard but i only quickly hacked this together :)
Ok raycasting. I am constantly raycast as i am doing a grid based system. whenever the mouse moves over a cell it calls the grid.getcell which gets the cell and provides info about whether the unit contains a player/enemy or is walkable. Also when the player is is selected i need to constantly cast the raycast to get this cell so that i can tell if its a walkable cell and i change the position of mousemarker to the raycast hit spot so that i can see the proposed path. If theres a better way to handle this to get a cell and its contents under the mouse cursor without an expensive raycall id love to hear it. i cant think of another way to get the cell details when the mouse moves into another "grid".. Please note i am using a terrain objects using a virtual grid based on the world co-ords. not a object made up of physical squares.
I figured out before i read your post that the render.color was a very bad idea :) fixed that one up. and the movement. i did have code that detects when it reaches the end i just didnt put it in original post as i find it very hard to post code into the text boxes here. the actual directio movement was copied from the sample of the a* pathfinding system i used. though i think i will be writing my own pathfinding soon as i am using a grid based system and a* probably isnt the best solution.
