Game broken using threads

Question

Hi!

I have some character nodes (area2d), which use the getsimplepath () function on a 200x200 tilemap.
I had problems, each node took time to calculate the path, and the main thread was blocking a few milliseconds, so I have tried using threads.

Now, if I put only one character, it works correctly, with 2 or 3, it takes a while to throw the error. And if for example I put 5 or more, throw error instantaneous. And the game breaks and closes.

The error message I get is:

E 0: 00: 08.518 get: FATAL: Index p_index = -1 is out of bounds
(size () = 4).
C ++ source ./core/cowdata.h:152 @ get ()

Character node code:

var speed : = 400.0
var path : = []

var thread = Thread.new()

func _ready() -> void:
    randomize()
    newPath()

func _unhandled_input(event: InputEvent) -> void:
    if not event is InputEventMouseButton:
        return
    if event.button_index != BUTTON_LEFT or not event.pressed:
        return


func _process(delta: float) -> void:
    if path.size() != 0:
        var move_distance : = speed * delta
        move_along_path(move_distance)
    else:
        newPath()


func move_along_path(distance : float) -> void:
    var last_point : = global_position
    for index in range(path.size()):
        var distance_to_next = last_point.distance_to(path[0])
        if distance <= distance_to_next and distance >= 0.0:
            global_position = last_point.linear_interpolate(path[0], distance / distance_to_next)
            break
        elif distance < 0.0:
            global_position = path[0]
            newPath()
            break

        distance -= distance_to_next
        last_point = path[0]
        path.remove(0)


func newPath():
    if (thread.is_active()):
        # Already working
        return
    print("START THREAD!")
    thread.start(self, "_on_newPath", get_instance_id())

func _on_newPath(id):
    print("THREAD FUNC!")
    print(id)
    var newPosition = Vector2(randi()%200, randi()%200)
    var tile_center_pos = get_parent().get_parent().get_child(0).get_child(0).map_to_world(newPosition) #map in tree
    var newPath = get_parent().get_parent().get_child(0).get_simple_path(global_position,tile_center_pos) #navigation2D node in tree
    #get_parent().get_child(2).points = newPath
    #newPath.remove(0)
    call_deferred("_on_newPath_done")
    return newPath


func _on_newPath_done():
    # Wait for the thread to complete, get the returned value
    var minionPath = thread.wait_to_finish()
    path = minionPath

The code without thread functions worked fine, with any number of nodes. I thing that the problem is with threads, but I cant understand the error message.

Comments

I don't know threading very well on Godot, but I think there may be an error due to access on shared resources. Maybe two or more threads try to access the same node. Read should not be a problem but maybe access enables also writing and in that case it may be a problem.

Try using mutex.lock() and mutex.unlock() around your getNodes.

---

Thanks, but have tried using mutex but it remains the same.
I have read in another post, that we should" NOT access the tree in a thread". But I don't understand how to calculate the Minion's path without accessing the tilemap and navigation nodes ..
I have cleaned up the code to pick up the map node before, but it didn't work either.

First get the nodes:

onready var Map = get_parent().get_parent().get_child(0).get_child(0)
onready var Nav = get_parent().get_parent().get_child(0)

And in thread:

var tile_center_pos = Map.map_to_world(newPosition)
var newPath = Nav.get_simple_path(global_position,tile_center_pos)

---

I have another question, this could also be useful.

You follow a path in _process function. You check length to avoid following an empty path and in that case you create a new one using threads...
But do you stop processing with set_process(false)?

Otherwise each character will start multiple threads or have unexpected behavior when path is calculated as processing will go on.

Instead try pausing process and restart once path is ready. Then you may not need threads at all.

---

I have tested what you say, stopping the process before calling the function that creates the path, and activating it after creating it.
Still, if I keep using threads, I get the same error, and if I don't use them, it hangs like before, a few milliseconds every time one of the minions recalculates the path. Thanks again!

---

it seems from the error you are trying to access element -1 of an array of size 4.

does the error point toward a specific line of the code?
if not, can you comment out parts of the code to pinpoint which function generate the error in particular?

---

Thx Andrea,

Ok, I have tried to detect where it fails in two ways:

Debugging line by line: The game does not fail. Works fine! But it's not a fun way to play xD
I think it will be because when going at normal speed, the threads cope with each other in some way, but I don't know how to solve it, or if is the real problem.

Adding many logs: Normaly stop in onnewPath function, but some times in newPath function. Never in the same lines.

---

yeah, it definitely look like a problem due to thread reding/writing onto the same variable.
you said you used mutex, are you sure you used them correctly?

although it is strange, cause your thread is not writing but only reading, unless get_simple_path and map_to_world are somehow also writing data.
does it change if you modify them to be normal variable? like

var tile_center_pos = random_vector_variable
var newPath = random_vector2_array

or

var tile_center_pos = get_node(whatevernode).random_vector_variable
var newPath = get_node(whatevernode).random_vector2_array

---

I think the problem with concurrent access is also due to the fact that since it is the scene tree every other node in code will access the same (and could read/write) so it would be necessary to mutex every possible access to it. (that I think it is impossible due to everything running in background of the engine).

So maybe the solution could be creating a local isolated representation of the map to compute paths. If the map is static there should be no problem.

---

Hi Andrea,

I had never used mutex, but following the official documentation I think I do it well.
At the beginning I create the mutex:

var mutex = Mutex.new()

And then I apply it here (I have also tried to apply it in other sites):

mutex.lock()
thread.start(self, "_on_newPath", get_instance_id())
mutex.unlock() 

I have also set the unlock at the end of the thread

func _on_newPath_done():
    # Wait for the thread to complete, get the returned value
    var minionPath = thread.wait_to_finish()
    path = minionPath
        mutex.unlock() 

It is right?

I have tried what you say, and it works!

var tile_center_pos = Map.map_to_world(newPosition)
print("map!")
var newPath = [Vector2(100,100),Vector2(550,550),Vector2(150,150),Vector2(170,170)]

So the problem is in getsimplepath. I have tried to surround only this line with mutex but it does not work either

---

Hi gioele!

The project for now is very simple, only have some nodes, nav2d, tilemap, camera, and minions.

The map is generated procedurally, and minions position too. I don't have other nodes that conect with map or Navigation2d..

I can put project in drive if you want for see the problem. It's very small.

---

what gioele said makes completely sense, and the fact that it works without get_simple_path is a sign of confirmation.

for the mutex part, i dont think that's the right way to do it (if i understood correctly, i'm not an expert either) you should lock it immediatly before accessing the navigation map and unlock it after you get the path.

something like

mutex.lock()
var tile_center_pos = tile_map_node.map_to_world(newPosition) #map in tree
var newPath = navigation_node.get_simple_path(global_position,tile_center_pos)
mutex.unlock()

(i think this is what gioele intended in his first comment) but it could also not be enough because the main thread can still access the nodes, making a mess. It's a start though!

---

Hey Andrea, yes, I was telling you at the end of my last comment, I already tried it with mutex on those lines, and nothing ..

So to make a game of this style, the solution would be to save the map information in an array or list for example, and update it every time the map is updated, and work with it? I find it strange that godot can't add multiple characters that move around the map to create an RTS-style game...

I also don't understand why mutex doesn't work in this case.

On the other hand, I could try instead of creating a Minion node for each instance, and each with the thread's code, create a script only for thread, and extend minion node with it. In this case, all of him enter in the same thread function, and maybe mutex works. What do you think?

---

i think this is not a problem with Godot, but a problem with multiple threads in general.

you can still share the simplified project, i'm really curious to see if we can find a way to make it work.
however i think you should also look for a workaround (something like what you mentioned), because having one thread for each minion is not super efficient: what happens if you have hundreds of minion on the scene? your pc will not be able to handle that amount of threads

---

I understand that it is not good to create threads for 100 nodes, but I don't know how to do it any other way, without stopping the main process.
I leave you a link to drive to download the project! It is not minimized, because it is already very small. :)
You will see that the map node at startup generates the map

func makeIsle():

and at the end of this funcrion add the minions:

for n in range (0,15): 

The Minions is a Human.tscn node, with Character.gd script.

The game does nothing else for now

I uploaded it to Github:
https://github.com/KenderM/Godot-game

Lot of thanks!

Answer 1

i think i understood what you were doing wrong with the mutex: you were (probably) creating a mutex for each character, instead of creating a single shared one.
if each character has his own mutex variable, when reaching the mutex.lock() line they will always proceed (no other character will ever lock their own mutex) genereting the cuncurrent access error.

what you want to do is creating a common one in a parent node: each character need to look at this mutex and lock/unlock before proceeding with the map access.
For example, in the Demo main node

var mutex
func _ready():
        mutex=Mutex.new()

and in the character

func _on_newPath(id):
    var newPosition = Vector2(randi()%200, randi()%200)
    get_node("/root/Demo").mutex.lock()
    var tile_center_pos = Map.map_to_world(newPosition)
    var newPath = Nav.get_simple_path(global_position,tile_center_pos) #navigation2D node in tree
    get_node("/root/Demo").mutex.unlock()
    call_deferred("_on_newPath_done")
    return newPath

i tried up to 200 character and it works, although the majority will remain still as my pc can manage only 12 threads at a time (considered the average path lenght and the time to calculate it, it is equivalent to roughly 20 character moving while the other were waiting for a free thread to start calculating their path)

Comments

i just realize that this code structure is more similar to a single thread that calculate the path of each character one after the other (there are multiple threads, but each one have to wait for the path calculation of the previous one).

to fully use each thread at everytime without queueing them, you probably have to save multiple representation of the map as suggested previously, and let each thread access only one of them (eg: 12 threads means 12 map representation)

---

It works!
Thank you very much for getting rid of this headache! :)

I can't process more than 12 +- either. But as in the future, when they arrive at the destination, they will be quiet for a while (good time for get the new path). Or I can even recalculate the next path while still walking if necessary. With a few tricks like that, I'm sure I can get a few more minions to work :)

Thanks again! I owe you a beer.