Unity - Refactored crumbling wall script stops wor

Question

I have an Object that gets replaced by thousands of little cubes at once, which then begin moving one after another after initialization.

I have code that works, but when I try to refactor it to clean it up, it stops working. The cubes dont move. This happens when I try to separate the Variable Initialisation and the Movement Initialisation.

So this is my original code segment and it works:

public class WallCreation : MonoBehaviour {

    public Transform wallSegmentPrefab;
    GameObject oldWall;
    Vector3 oldWallSize;
    int oldWallsizeX;
    int oldWallsizeY;
    int oldWallsizeZ;
    Vector3 oldWallPosition;
    Vector3 oldWallCornerPosition;
    Transform newWall;
    Transform parentWallSegment;
    Transform[ , , ] wallSegments;
    int[] indizes;

void Start () { 
    indizes= new int[3];
}

public void newWallScript(){
    initializeNewWall ("zWall++");
    StartCoroutine (waitForMovement ());
}

void initializeNewWall(string replaceWall)
{
    oldWall = GameObject.Find(replaceWall);
    oldWallSize = oldWall.transform.localScale;
    oldWallPosition = oldWall.transform.localPosition;
    oldWallsizeX=(int) oldWallSize.x;
    oldWallsizeY=(int) oldWallSize.y;
    oldWallsizeZ=(int) oldWallSize.z;
    oldWallCornerPosition = oldWallPosition - oldWallSize / 2 + wallSegmentPrefab.localScale / 2;

    wallSegments = new Transform[oldWallsizeX , oldWallsizeY , oldWallsizeZ];

    for (int x = 0; x < oldWallsizeX; x++)
    {           
        for (int y = 0; y < oldWallsizeY; y++)
        {
            for (int z = 0; z < oldWallsizeZ; z++)
            {
                newWall = Instantiate(wallSegmentPrefab);

                GameObject _wallSegment = newWall.gameObject;
                _wallSegment.AddComponent<WallMovement> ();
                wallSegments[x,y,z] = newWall;
            }
        }
    }
    oldWall.SetActive(false);
}

void newWallMovement()
{
    for (int x = 1; x < oldWallsizeX-1; x++)
    {
        indizes [0] = x;                   
        for (int y = 0; y < oldWallsizeY; y++) 
        {
            indizes [1] = y;
            for (int z = 0; z < oldWallsizeZ; z++) {
                indizes[2] = z;

                newWall = wallSegments[x,y,z];
                GameObject _wallSegment = newWall.gameObject;
                WallMovement _WallMovement = _wallSegment.GetComponent<WallMovement> ();
                _WallMovement.indizes = indizes;

                _WallMovement.initializeMovement ();

            }
        }
    }
}

IEnumerator waitForMovement()
{
    yield return new WaitForSeconds(1f);
    newWallMovement();
}

}

This is my improved code that does not work and (...) stays the same:

public class WallCreation : MonoBehaviour {

//(...) 

public void newWallScript(){
    //(...)
    StartCoroutine (waitForMoving());
}

void initializeNewWall(string replaceWall)
{
    (...)
}

void newWallMovement()
{
    for (int x = 1; x < oldWallsizeX-1; x++)
    {
        indizes [0] = x;                   
        for (int y = 0; y < oldWallsizeY; y++) 
        {
            indizes [1] = y;
            for (int z = 0; z < oldWallsizeZ; z++) {
                indizes[2] = z;

                newWall = wallSegments[x,y,z];
                GameObject _wallSegment = newWall.gameObject;
                WallMovement _WallMovement = _wallSegment.GetComponent<WallMovement> ();
                _WallMovement.indizes = indizes;

                //this is cut out and put into the wallMoving() void
                //_WallMovement.initializeMovement ();

            }
        }
    }
}

void wallMoving(){
    for (int x = 1; x < oldWallsizeX-1; x++)
    {
        //indizes [0] = x; //only with this enabled it works for some reason, otherwise it doesn't                    
        for (int y = 0; y < oldWallsizeY; y++) 
        {
            for (int z = 0; z < oldWallsizeZ; z++) {
                newWall = wallSegments[x,y,z];
                GameObject _wallSegment = newWall.gameObject;
                WallMovement _WallMovement = _wallSegment.GetComponent<WallMovement> ();

                //same code but without giving the list indizes[] to the script/gameObject
                _WallMovement.initializeMovement ();
            }
        }
    }
}
IEnumerator waitForMovement()
{
    (...)
}
IEnumerator waitForMoving()
{
    yield return new WaitForSeconds(1f);
    wallMoving();
}

}

When I separate this line _WallMovement.initializeMovement (); to another function, the game continues to work, but the wall doesnt move this time. The Indizes seem to not be initialized anymore. This however does not result in an error in the console.

Here is some additional code from my script:

This is what happens in the WallMovement script, that got attached to every cube of the wall:

public class WallMovement : MonoBehaviour {
public int[] indizes ;
int indize;

int modulo;

public void initializeMovement()
{
    modulo = indizes [0] % 2; 
    if (modulo>0) 
    {           
        //do something
    } 
    else 
    {
        // do something else
    }
}

}

Answer 1

First of all do not mess with anything you don't have to until you actually get your refactored code working. Otherwise you will be trying to fix multiple problems.

void newWallMovement()
    {
        for (int x = 1; x < oldWallsizeX-1; x++)
        {
            indizes [0] = x;

            for (int y = 0; y < oldWallsizeY; y++) 
            {
                indizes [1] = y;

                for (int z = 0; z < oldWallsizeZ; z++) {
                    indizes[2] = z;

                    newWall = wallSegments[x,y,z];
                    GameObject _wallSegment = newWall.gameObject;
                    WallMovement _WallMovement = _wallSegment.GetComponent<WallMovement> ();
                    _WallMovement.indizes = indizes;
                }
            }
        }
    } 

Leave everything the same and only remove _WallMovement.initializeMovement ();.

Now you will have to call the same function, somewhere else. This is incredibly inefficient because you now have to iterate through every wall position AGAIN, instead of doing everything at once.

Here is an example.

Private List<WallMovement> WallCollection = new List<WallMovement>();
private WallMovement newWallMovement()
    {
        for (int x = 1; x < oldWallsizeX-1; x++)
        {
            indizes [0] = x;

            for (int y = 0; y < oldWallsizeY; y++) 
            {
                indizes [1] = y;

                for (int z = 0; z < oldWallsizeZ; z++) {
                    indizes[2] = z;

                    newWall = wallSegments[x,y,z];
                    GameObject _wallSegment = newWall.gameObject;
                    WallMovement _WallMovement = _wallSegment.GetComponent<WallMovement> ();
                    _WallMovement.indizes = indizes;
                    WallCollection.Add(_WallMovement);
                }
            }
        }
    } 

When you call these two functions you can do something like this

IEnumerator waitForMovement()
{
   newWallMovement();
   yield return new WaitForSeconds(1f);
   InializeAllWallMovement();
}

private void InializeAllWallMovement()
{
   foreach(WallMovement wm in WallCollection)
   {
      wm.initializeMovement(); 
   }
}

As you can see this is not actually an improvement, but makes your code more complicated and take longer. The way it was before was probably about as simplified as this is going to get. If you must move that code outside because it is necessary for some reason, then try something like what I suggested.

Answer 2

My mistake is, that I pass the indize by reference and not by value to the wallscripts.

Therefore when I change the indizes in the main script they get changed in every wall script. So when I call the public functions of the wall later, they all use the same indizes, the last ones I initialized, and therefore are not effected by my "Initialization" in the first place.

Otherwise if I change the indizes values before I call the wall function it works again, because the script now uses the correct corresponding values, so to correct that I need to pass the indizes one by one by value so that they are all initialized for real with their corresponding value.

Thanks go out to https://catlikecoding.com/unity/tutorials/ for looking over this really stupid mistake and tellig me what no one else here could. At least he didnt try to correct my code for being slow.

So this is my new code segment that works, but is inefficient, still it does exactly what I wanted: For better readability I dont write all the code that worked again. It is marked with (...) and stays the same

public class WallCreation : MonoBehaviour {

    (...) //stays the same as in the working code

    void Start () { 
        indizes= new int[3];
    }

    public void newWallScript(){
        initializeNewWall ("zWall++");
        StartCoroutine (waitForMovement ());
        StartCoroutine (waitForMoving()); //the new coroutine with a later start is added
    }

    void initializeNewWall(string replaceWall)
    {
        (...) //stays the same
    }

    void newWallMovement()
    {
        for (int x = 1; x < oldWallsizeX-1; x++)
        {
            indizes [0] = x;                   
            for (int y = 0; y < oldWallsizeY; y++) 
            {
                indizes [1] = y;
                for (int z = 0; z < oldWallsizeZ; z++) {
                    indizes[2] = z;

                    newWall = wallSegments[x,y,z];
                    GameObject _wallSegment = newWall.gameObject;
                    WallMovement _WallMovement = _wallSegment.GetComponent<WallMovement> ();

                    //_WallMovement.indizes = indizes; //this is a 'passing by reference', because it is an array

                    _WallMovement.indizes[0] = indizes[0]; //these pass the parameter by value, you could also just say _WallMovement.indizes[0] = x; etc.
                    _WallMovement.indizes[1] = indizes[1];
                    _WallMovement.indizes[2] = indizes[2];

                    //this is cut out and put into the wallMoving() void
                    //_WallMovement.initializeMovement ();

                }
            }
        }
    }

    void wallMoving(){
        for (int x = 1; x < oldWallsizeX-1; x++)
        {                  
            for (int y = 0; y < oldWallsizeY; y++) 
            {
                for (int z = 0; z < oldWallsizeZ; z++) {
                    newWall = wallSegments[x,y,z];
                    GameObject _wallSegment = newWall.gameObject;
                    WallMovement _WallMovement = _wallSegment.GetComponent<WallMovement> ();

                    _WallMovement.initializeMovement ();
                }
            }
        }
    }
    IEnumerator waitForMovement()
    {
        (...) //stays the same
    }
    IEnumerator waitForMoving()
    {
        //the time to wait has no consequence on performance whatsoever
        yield return new WaitForSeconds(1f);
        wallMoving();
    }
}

Here is some additional code from my script:

This is what happens in the WallMovement script now, that got attached to every cube of the wall:

public class WallMovement : MonoBehaviour {
    public int[] indizes ;
    int indize;

    int modulo;
void Awake (){
        indizes = new int[3]; // this is added, because we dont pass the list, but the single values, so it needs to be declared as a 3-dimensional array inbefore
    }

    public void initializeMovement()
    {
        modulo = indizes [0] % 2; 
        if (modulo>0) 
        {           
            //do something
        } 
        else 
        {
            // do something else
        }
    }
}