My instantiated rooms keep overlapping with each other.

Question

I am trying to make a room generator and i can't figure out how to fix this problem.

Main Node:
`public Vector2 right = new Vector2(464,0);
public Vector2 left = new Vector2(-464,0);
public Vector2 up = new Vector2(0,-256);
public Vector2 down = new Vector2(0,256);

public PackedScene room1 = ResourceLoader.Load<PackedScene>("res://Scenes/room_1.tscn");
public PackedScene room2 = ResourceLoader.Load<PackedScene>("res://Scenes/room_2.tscn");

public override void _Ready()
{
for (int i = 0; i < 4; i++)
{
var room = GD.Randi() % 2 + 1;

			var roomInstance = room1.Instantiate<Node2D>();
			
			if (room == 1)
			{
				var direction =  GD.Randi() % 4 + 1;
				
				if (direction == 1)
				{
					roomInstance.Position = right;
				}
				else if (direction == 2)
				{
					roomInstance.Position = left;
				}
				else if (direction == 3)
				{
					roomInstance.Position = up;
				}
				else if (direction == 4)
				{
					roomInstance.Position = down;
				}
				rooms.AddChild(roomInstance);
			}
			else if (room == 2)
			{
				var direction =  GD.Randi() % 4 + 1;
				
				var roomInstance2 = room2.Instantiate<Node2D>();
				
				if (direction == 1)
				{
					roomInstance2.Position = right;
				}
				else if (direction == 2)
				{
					roomInstance2.Position = left;
				}
				else if (direction == 3)
				{
					roomInstance2.Position = up;
				}
				else if (direction == 4)
				{
					roomInstance2.Position = down;
				}
				rooms.AddChild(roomInstance2);
			}
	}

}`

Tobacconist

Answer 1

Where does the rooms come from where you add the children to? I can't see it in your script.

Rifle

Answer 2

"rooms" is a Node2D which only serves to be a parent to these rooms.

Tobacconist

Answer 3

Aren’t you assigning absolute positions to each room rather than the room position * the loop iteration, which I think is your intention?

Slow

Answer 4

For each room you get a random direction, so it is possible that some rooms get the same direction/position.
Also you create a roomInstance BEFORE you check if you want room 1 or 2. If it is 2 then you will instantiate room 1 and 2.

Rifle

Answer 5

I managed to make it work like this:
`for (int i = 0; i < num_levels; i++)
{
var room = GD.Randi() % 2 + 1;

			if (room == 1)
			{
				var direction =  GD.Randi() % 8 + 1;
				var roomInstance = room1.Instantiate<Node2D>();
				rooms.AddChild(roomInstance);
				
				if (direction == 1)
				{
					rightTimes += 1;
					roomInstance.GlobalPosition = right * rightTimes;
				}
				else if (direction == 2)
				{
					leftTimes += 1;
					roomInstance.GlobalPosition = left * leftTimes;
				}
				else if (direction == 3)
				{
					upTimes += 1;
					roomInstance.GlobalPosition = up * upTimes;
				}
				else if (direction == 4)
				{
					downTimes += 1;
					roomInstance.GlobalPosition = down * downTimes;
				}
				else if (direction == 5)
				{
					leftUpTimes += 1;
					roomInstance.GlobalPosition = leftUp * leftUpTimes;
				}
				else if (direction == 6)
				{
					leftDownTimes += 1;
					roomInstance.GlobalPosition = leftDown * leftDownTimes;
				}
				else if (direction == 7)
				{
					rightUpTimes += 1;
					roomInstance.GlobalPosition = rightUp * rightUpTimes;
				}
				else if (direction == 8)
				{
					rightDownTimes += 1;
					roomInstance.GlobalPosition = rightDown * rightDownTimes;
				}
				GD.Print("Room1");
				previous_room = roomInstance;
			}
			else if (room == 2)
			{
				var direction =  GD.Randi() % 8 + 1;
				
				var roomInstance2 = room2.Instantiate<Node2D>();
				rooms.AddChild(roomInstance2);
				
				if (direction == 1)
				{
					rightTimes += 1;
					roomInstance2.GlobalPosition = right * rightTimes;
				}
				else if (direction == 2)
				{
					leftTimes += 1;
					roomInstance2.GlobalPosition = left * leftTimes;
				}
				else if (direction == 3)
				{
					upTimes += 1;
					roomInstance2.GlobalPosition = up * upTimes;
				}
				else if (direction == 4)
				{
					downTimes += 1;
					roomInstance2.GlobalPosition = down * downTimes;
				}
				else if (direction == 5)
				{
					leftUpTimes += 1;
					roomInstance2.GlobalPosition = leftUp * leftUpTimes;
				}
				else if (direction == 6)
				{
					leftDownTimes += 1;
					roomInstance2.GlobalPosition = leftDown * leftDownTimes;
				}
				else if (direction == 7)
				{
					rightUpTimes += 1;
					roomInstance2.GlobalPosition = rightUp * rightUpTimes;
				}
				else if (direction == 8)
				{
					rightDownTimes += 1;
					roomInstance2.GlobalPosition = rightDown * rightDownTimes;
				}`

Thank you for your suggestions.

Tobacconist

Answer 6

This many ifs will make your computer malfunction 😉 Organize and structure your room/direction data into arrays and dictionaries and let it drive the code. This will immensely reduce the amount of needed code and make the whole thing scalable. Think about it. What if you in future would want to add ten more room types to this system? Repeating the same 30 lines of code ten more times is not a very clever way to go about it.

Felton

Answer 7

Wait, really? Thanks for the advice!

Tobacconist

Answer 8

I was obviously joking about "malfunction" but excessive and repetitive ifing like this is typically a sign of poor coding practice and an open invitation to bugs.

Better to turn all these conditions into data points and iterate/index them in a couple of lines of code. It'll result in code that's much shorter yet more versatile as it can be extended/altered simply by supplying more/different data.

Coding is automation. If you need to repeatedly type in same (or similar) things over and over, you're not doing a very good job at automation.

Felton

Answer 9

I was obviously joking about "malfunction" but excessive and repetitive ifing like this is typically a sign of poor coding practice and an open invitation to bugs.

insert here something about modern cpu's branch predictors and branch misses

Tyro

Answer 10

I wanted to post this earlier, but I was busy. Like xyz said your code structure is no ideal and can be simplified and be made more flexible.
I have an example, but beware: it is in GDScript and for 3D positioning (In 2D there is no Vector Forward).

Also I'm not sure if C# has the pick_random method.
Maybe someone can tell me if Godot's C# has all the same methods like GDscript has?

I just wanted to show an approach to this and there are probably better and/or other solutions.
I strongly recommend you to look at loops, array and dictionaries. They make life a lot easier. 🙂

extends Node

@export var min_room_count := 1
@export var max_room_count := 10

@export var room_scenes : Array[PackedScene] # Array that contains all available room scenes
@onready var rooms_parent: Node3D = $RoomsParent


# currently only 4 directions, but you could add diagonal directions too
var directions = [
	Vector3.FORWARD,
	Vector3.RIGHT,
	Vector3.BACK,
	Vector3.LEFT
	]

func _ready() -> void:
	var target_room_count = randi_range(min_room_count, max_room_count)
	
	for i in target_room_count:
		var room_instance = room_scenes.pick_random().instantiate()
		var position = directions.pick_random()
		
		#TODO: Store existing rooms in a dictionary or array and check if on this position rooms exist
		#TODO: If there is already a room apply an offset to your position and check again.
		
		room_instance.position = position
		rooms_parent.add_child(room_instance)

Rifle