koobas.hobune.stream
Simplify this code
Here's a coroutine, it has two very similar loops that lerps the cam.LerpX or cam.LerpY depending on the CameraBoundary.Mode, the code works fine but whenever I see repeating lines of code it usually means that there is room for improvement, can this be simplified to a single loop? I don't understand how to have a variable that references either cam.LerpX or cam.LerpY.
IEnumerator TransitionCoroutine(CameraBoundary.Mode mode, bool transitionIn)
{
float time = transitionIn ? transitionTime : -transitionTime;
float lerpValue = transitionIn ? 1 : 0;
if (mode == CameraBoundary.Mode.Vertical)
{
while (cam.lerpX != lerpValue)
{
if(cam.lerpX == 1)
yield return new WaitForSeconds(0.1f);
if (controller.transitioning)
yield break;
cam.lerpX = Mathf.Clamp01(cam.lerpX + time * Time.deltaTime);
yield return null;
}
}
else if (mode == CameraBoundary.Mode.Horizontal)
{
while (cam.lerpY != lerpValue)
{
if(cam.lerpY == 1)
yield return new WaitForSeconds(0.1f);
if (controller.transitioning)
yield break;
cam.lerpY = Mathf.Clamp01(cam.lerpY + time * Time.deltaTime);
yield return null;
}
}
}
I don't know about the loop but your yield return new WaitForSeconds(0.1f); is not optimised if this Coroutine is called multiple times. Ins$$anonymous$$d you can use
WaitForSeconds waitOn$$anonymous$$illiSec;
void Start()
{
waitFor$$anonymous$$illiSec = new WaitForSeconds(0.1f);
}
and now you can use yield return waitFor$$anonymous$$illiSec; ins$$anonymous$$d of yield return new WaitForSeconds(0.1f); everytime
In this way it will not create a new class of WaitForSeconds every time this coroutine is called.
Thanks, the coroutine is not called often but I didn't know about caching WaitForSeconds, interesting
I've decided to go with what Strotosome and Sonky108 have suggested:
IEnumerator TransitionCoroutine(CameraBoundary.Mode mode, bool transitionIn)
{
float time = transitionIn ? transitionTime : -transitionTime;
float lerpValue = transitionIn ? 1 : 0;
float lerpTarget = mode == CameraBoundary.Mode.Vertical ? cam.lerpX : cam.lerpY;
WaitForSeconds waitForMillisecond = new WaitForSeconds(0.1f);
while (lerpTarget != lerpValue)
{
if (lerpTarget == 1)
yield return waitForMillisecond;
if (controller.transitioning)
yield break;
lerpTarget = Mathf.Clamp01(lerpTarget + time * Time.deltaTime);
if (mode == CameraBoundary.Mode.Vertical)
cam.lerpX = lerpTarget;
else
cam.lerpY = lerpTarget;
yield return null;
}
}
Thanks for the suggestions!
It is a very good instinct to react to repeating lines with "that could be improved" - it usually is true. In this case a simple factoring out of the block of the branch and eliminating differences with a common variable should do it:
private IEnumerator TransitionCoroutine (CameraBoundary.Mode mode, bool transitionIn) {
float time = transitionIn ? transitionTime : -transitionTime;
float lerpValue = transitionIn ? 1 : 0;
if (mode == CameraBoundary.Mode.Vertical) { yield return LerpRoutine(cam.lerpX, lerpValue); }
else if (mode == CameraBoundary.Mode.Horizontal) { yield return LerpRoutine(cam.lerpY, lerpValue); }
}
private IEnumerator LerpRoutine (float camLerp, float lerpValue) {
while (camLerp != lerpValue) {
if (camLerp == 1)
yield return new WaitForSeconds(0.1f);
if (controller.transitioning)
yield break;
camLerp = Mathf.Clamp01(cam.lerpY + time * Time.deltaTime);
yield return null;
}
}
Some further possible improvements:
- instead of if-else-if-else... chain use a switch-case for branching on the value of CameraBoundary.Mode (I'm assuming it is an enum)
- as Piyush_Pandey said, cache the wait object (common type is YieldInstruction), even if just as a local variable before the while loop
I've already chosen a solution but thanks for the suggestion, I appreciate it, always happy to discover new ways of doing things
Without repeating code you try something like this:
IEnumerator TransitionCoroutine(CameraBoundary.Mode mode, bool transitionIn)
{
float time = transitionIn ? transitionTime : -transitionTime;
float lerpValue = transitionIn ? 1 : 0;
float lerpTarget;
if (mode == CameraBoundary.Mode.Vertical)
{
lerpTarget = cam.lerpX;
}
else
{
lerpTarget = cam.lerpY;
}
while (lerpTarget != lerpValue)
{
if(lerpTarget == 1)
yield return new WaitForSeconds(0.1f);
if (controller.transitioning)
yield break;
lerpTarget = Mathf.Clamp01(lerpTarget + time * Time.deltaTime);
yield return null;
}
}
The problem is that this is just going to set lerpTarget and not my camera's LerpX and LerpY variables, cam is my camera script and I'm trying to set the two variables in that script during the loop, lerpTarget would have to be a direct reference to one of those variables, I'm not sure if this is possible, thanks for giving it a try!
I was thinking about something like that too, but that isn't actually updating the cam.lerpX or cam.lerpY variables (like $$anonymous$$henaB said). I guess this could be added right before the yield return null:
if (mode == CameraBoundary.$$anonymous$$ode.Horizontal) {
cam.lerpX = lerpTarget;
}
else if (mode == CameraBoundary.$$anonymous$$ode.Vertical) {
cam.lerpY = lerpTarget;
}
$$anonymous$$aybe?
That works, it's slightly harder to read but I prefer that over the two loops, thank you both for the suggestions.
