8
\$\begingroup\$

The idea here is that rather than placing objects directly into the scene, these building objects are instantiated at run time and placed in the correct locations. This gives me full control over where and how to place the buildings.

The workflow is still a little involved. I have to import the model and texture image, drag it out into the scene, scale it, add a Mesh collider to it, and then add all of that to a prefab object that I also need to create. Then I need to add the name of that prefab to the script below. This must be done for each model that I want to use in the game. This approach is still a whole lot better than having to manually place and position each object in the Unity scene though.

GameModelScript.cs

using UnityEngine;
using System;
using System.Collections;
using System.Collections.Generic;

public class GameModelScript : MonoBehaviour {

    public int worldSize;

    private System.Random random = new System.Random();

    void Start () {
        List<string> names = new List<string> ();
        names.Add ("arco01_2_prefab");
        names.Add ("arco03_prefab");
        names.Add ("arco04_prefab");
        names.Add ("arco05_prefab");
        names.Add ("ballTower01_prefab");
        names.Add ("bank01_prefab");
        names.Add ("barn01_prefab");
        names.Add ("bigApartments05_prefab");
        names.Add ("bigFactory06_prefab");
        names.Add ("bigMatterCube01_prefab");
        names.Add ("busDepot01_prefab");
        names.Add ("casino01_prefab");

        for (int y = 0; y < worldSize; y+=30) {
            for (int x = 0; x < worldSize; x+=30) {
                int randomNumber = random.Next(0, names.Count);
                GameObject tileObject = (GameObject)GameObject.Instantiate (Resources.Load (names[randomNumber]));
                tileObject.transform.position = new Vector3(x, 10, y);
            }
        }
    }

    void Update () {
    }
}

My plan is to use this foundation to build some kind of superhero city exploration game. Here is a link to a very early playable version: Super Voxel

And here is a little screenshot from up high:

enter image description here

\$\endgroup\$
  • \$\begingroup\$ Do you have any plans for this? Like a game, or something? Or is this just a test? Very cool by the way ;-) \$\endgroup\$ – Ethan Bierlein Aug 16 '15 at 2:24
  • 1
    \$\begingroup\$ @EthanBierlein see this question for something a little more game like: codereview.stackexchange.com/q/101324/14608 \$\endgroup\$ – bazola Aug 18 '15 at 22:14
8
\$\begingroup\$

Unlike Java Lists, C# lists can be pre-initialized with values, like this:

List<string> names = new List<string>() 
{
    "arco01_2_prefab",
    ...
};

This eliminates the need to call .Add, and improves code readability as well.

In the future as well, if you ever plan to add more buildings, it'll be a pain to edit this script to include the new building names. Instead, you could declare a public variable which allows you to drag and drop the model prefabs instead:

public List<GameObject> buildings;

This also eliminates the need to call Resources.Load.

In addition, the namespace UnityEngine provides a class Random which can be used over the default System.Random. Unlike System.Random, UnityEngine.Random is a static class doesn't need to be instantiated, and you can just call Random.Range like this:

int randomNumber = (int)Random.Range(0, names.Count);

Other than that, there are just a few things I want to nitpick.

  • You don't need to call Instantiate from the GameObject class. It can just be called with out the GameObject. prefix.
  • In addition, the accepted Unity style for storing an instantiated GameObject is like this:

    GameObject gameObject = Instantiate( ... ) as GameObject;
    
  • If you're feeling up to dynamic typing, this also works as well (Thanks @Kroltan):

    var gameObject = Instantiate( ... ) as GameObject;
    
  • You can also remove the Update function. It doesn't do anything, and can decrease performance, due to the fact that it's running many times per frame.

  • The general style for braces in C# is to put them on the next line, not like Java braces.
  • You aren't using System.Collections, so you can remove the using System.Collections;.
\$\endgroup\$
  • 1
    \$\begingroup\$ Or even better, var gameObject = Instantiate( ... ) as GameObject. Since there is already a cast, it might look cleaner to infer the type of the variable itself, considering that var is a purely compile-time construct with no overhead. \$\endgroup\$ – Kroltan Aug 14 '15 at 2:55
4
\$\begingroup\$

Just as a note ahead of time, I don't know much about C# myself so my review will be fairly short.


Rather than creating a new List and then creating adding all the entries you need afterwards, a cleaner (and possibly faster) solution would be to...

  1. Create an array of all the entries you'd like to add.

  2. Use ToList to create a new list.

That would look something like this:

string[] prefabs = new string[] {"arco01_2_prefab", ...};
List<string> names = prefabs.ToList();

You have a few magic numbers in your code:

for (int y = 0; y < worldSize; y+=30) {

and:

tileObject.transform.position = new Vector3(x, 10, y);

Where did the 30 come? And the 10?

I recommend that you create to constants to hold these values, and then to use the constants instead of the normal number.

That might look like this (in your class):

private const int BUILDING_SPACING = 30;
private const int BUILDING_HEIGHT = 10;

Note that if you want to edit these in the Unity editor, you will have to make these public and no longer constants. Thanks Ethan Bierlein.

Then, in your code, you would just use BUILDING_SPACING and BUILDING_HEIGHT as needed.

Note: You already have a constant worldSize. This should be created with const.

\$\endgroup\$
  • \$\begingroup\$ The two variables BUILDING_SPACING and BUILDING_HEIGHT should be private, unless @bazola wants them to be modifiable through the Unity Editor, in which case, they shouldn't be const. \$\endgroup\$ – Ethan Bierlein Aug 16 '15 at 1:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.