koobas.hobune.stream
C# Creating a More Efficient Script
This has been driving me nuts. So I want to create a script that makes a gameobject rotate. I was able to achieve this but I still think this could be done better. I initially tried creating a vector2, assigning values to it and making the axes I don't want to move 0. But with Mathf.PingPong it gave me an error saying the transform.position attempt was not valid. Do you guys have any suggestions for improvements of my script?
using UnityEngine;
using System.Collections;
public class Ping_Pong_Sway : MonoBehaviour {
public float sway;
public string swayAlongVector;
void Update() {
if(swayAlongVector == "x"){
transform.localPosition = new Vector3(Mathf.PingPong(Time.time * 5, sway), transform.localPosition.y, transform.localPosition.z);
}
else if(swayAlongVector == "y"){
transform.localPosition = new Vector3(transform.localPosition.x, Mathf.PingPong(Time.time * 5, sway), transform.localPosition.z);
}
else if(swayAlongVector == "x+y"){
transform.localPosition = new Vector3(transform.localPosition.x, Mathf.PingPong(Time.time * 5, sway), transform.localPosition.z);
}
}
}
I have no idea what you're asking. Could you include some screenshots of what you're expecting vs. what you're seeing?
Also, if it gives you an error message, post it here. That helps us diagnose the problem a lot easier.
I'm looking for a better way of creating my script. It calls transform.localPosition 3 times and uses what I think is an extra variable in swayAlongVector. I haven't found an alternative to this however.
If your just trying to make it rotate in place (not around anything) you can just do something like this
tf.Rotate(-Vector2.up * rotateSpeed * Time.deltaTime);
You could use a switch statement
You just want to refactor your code?
First, I think you have a bug, you do the same for "y" and for "x+y".
Second, I guess you could avoid most of that repetition writing it like this:
using UnityEngine;
using System.Collections;
public class Ping_Pong_Sway : MonoBehaviour {
public float sway;
public string swayAlongVector;
void Update() {
//this check is not neccesary if swayAlongVector will alywas have one of this values
if (swayAlongVector == "x" || swayAlongVector == "y" || swayAlongVector == "x+y") {
float aux = Mathf.PingPong(Time.time * 5, sway); //write this only once
Vector3 pos = Vector3.zero;
pos.z = transform.localPosition.z;
if(swayAlongVector == "x" || swayAlongVector == "x+y"){
pos.x = aux;
}
if(swayAlongVector == "y" || swayAlongVector == "x+y"){
pos.y = aux;
}
transform.localPosition = pos;
}
}
}
That code should work like your code.
Another improvement would be to use an enum instead of strings for the swayAlongVector variable. And would be even better if the enum is a "flags" enum: http://www.dotnetperls.com/enum-flags
EDIT: Anyway, this code is not more "efficient", that's something you have to measure, but with something so simple it's probably an insignificant difference. Your original code doesn't create useless variables nor call the same function more than once, on each update only one of the conditions will be true, so only one line of code will get called. You should aim for cleannes, and maintainability instead of efficience in this case.
For example, it's more maintainable if you write that PingPong line only once in case you have to change something (change in one place instead of 3). It's easier to understand what the code does if you change the localPosition once at the end and use the swayAlongVector to only change the x and y values you'll use. But it's all subjective, someone might think other options are better for other reasons.
Just like to add that your refactored code does not the same as the original. If "swayAlongVector" is either "x" or "y" you set the y or x position to 0 as you initialized your pos with Vector3.zero. You should have initialized pos with localPosition.
It's true that calculating the pingpong value only in one place is more maintainable since each case will need the value anyways.
However swayAlongVector is just a selector (which would be better as enum, as you said) which is considered constant for an instance. In such a case i wouldn't mix the different cases as it makes it more entangled.
Also even you merged the pingpong calculation and the assignment into one place, you duplicated the comparing of the string values. If you want to add for example 3 "rotate" cases as well and in that run you rename the parameters from "x" ro "mx" for "move x" and add "rx" for "rotate x" your code is less maintainable.
Finally your script executes 4(best case) up to 7(worst case) string compare operations while the original code executes 1(best case) up to 3(worst case).
To me your solution is less readable and actually less efficient ^^
I would suggest using an enum like this:
public enum SwayOperation { X, Y, XY };
public float sway;
public float speed = 5f;
public SwayOperation swayType;
void Update()
{
float val = $$anonymous$$athf.PingPong(Time.time * speed, sway);
Vector3 pos = transform.localPosition;
switch (swayType)
{
case SwayOperation.X:
pos.x = val;
break;
case SwayOperation.Y:
pos.y = val;
break;
case SwayOperation.XY:
pos.x = pos.y = val;
break;
}
transform.localPosition = pos;
}
In the hypothetical case of adding more "operations" i would even move the assignment of localPosition into each case. It doesn't matter how long the code is as long as you can easily follow what it does. Also maintainability can't be generalized. You can consider something like "what might get changed more likely?", but you never know what might be changed in the future. If you know it already then you usually implement it right away ^^.
Make Sure that you didn't set the sway value to be 0. Mathf.PingPong(float t, float length); the parameter require to be bigger than 0.
