Portponky / better-terrain

Terrain plugin for Godot 4
The Unlicense
433 stars 21 forks source link

Invalid get index when updating batches via threads #66

Closed griessbreilp closed 6 months ago

griessbreilp commented 6 months ago

Am not quite sure if this is an actual problem with Better Terrain or with my code! I really enjoy Better Terrain, it already helped me quite a lot and is imo way better than the default that Godot has. Thank you really much for the plugin!

image So what happens is I create a noise filter, loop through two FORs for X and Y coordinates, calculate if they are too far away from my radius, and if not, check which noise I have on this Coord, and set a tile accordingly.

That leads to me being able to create circular (or which ever shape I want) islands with different terrains. To make it more performant I only update the layers when the whole island is calculated. I have overall ~6.400 - 14.400 Tiles I want to set and after that update.

It looks a bit like that:

Because I use the same array and layer_id for update_terrain_cells as I do for set_cells, I am pretty sure that the tile exists. image

When I open up a thread for each layer at once with "update_surrounding_cells" = false OR true (both cases) it fails: image It is btw not the same layer. Sometimes it is the third, sometimes the seventh, ... that doesn't seem to be the problem.

When I open up a thread for each layer after each other with "updating_surrounding_cells" = false it fails: image

When I open up a thread for each layer after each other with "updating_surrounding_cells" = true it succeeds: image

How I start the threads: image

How I kill the threads: image

I just change if !tile_batches_to_update.is_empty() && !tile_batches_updating: to if !tile_batches_to_update.is_empty(): to start multiple threads at once

It could totally be that my thread work is crap and that this causes the error, but I don't think that it should crash at all, even if there is a problem.

Portponky commented 6 months ago

The Godot Tilemap has serious problems when updating tiles outside of the main thread. Sometimes it works, sometimes it doesn't. in Better Terrain, the update_terrain_cells function calls Tilemap.set_cell, so it also can't be used outside of the main thread.

This isn't a huge problem because Better Terrain offers a threading system that will calculate the terrain in a thread pool, so it can be applied relatively quickly on the main thread.

Check out create_terrain_changeset, is_terrain_changeset_ready and apply_terrain_changeset functions.

griessbreilp commented 6 months ago

Mate, you are just nuts! Really appreciate it :D Thanks for the fast reply!

So if I understand this correctly, I still have to set the tiles with BetterTerrain.set_cells or what ever function on the Main-Thread. When the tiles are set I can use BetterTerrain.create_terrain_changeset with the Tilemap, the Layer ID and a Dictionary containing Coords to Tile ID. Is the Tile ID correct? Or what is "terrain types"?

This returns a Dictionary containing all the infos and the WorkerThreadPool Group Task. Which, if I might say, is just genious. And with that I can check if the _update_tile_deferred is done with all the tiles by calling is_terrain_changeset_ready and just passing the whole Dictionary.

If that returns true, I can apply it with apply_terrain_changeset and save the Dictionary for later use, which comes in really handy!

I do the following now:

Which works great so far! EXCEPT one error I get randomly. Every third or fourth time I get : image

in _update_tile_deferred types: Array doesn't contain my coord, so it runs into an error. I can prevent that by changing the line to: var type if types.has(coord): type = types[coord] else: return null

Am just wondering why this is happening?

Portponky commented 6 months ago

You don't need to call set_cells, the changeset takes a dictionary of Vector2i coordinates, and integer terrain types. If you check the documentation (via F1 or in BetterTerrain.gd) it shows what to do. There's also an example in a video I made.

So you can skip the set the tiles via better_terrain.set_cells on the main thread step. Just create the dictionary, and create the changeset from the main thread.

griessbreilp commented 6 months ago

I solved my problem. And no, it wasn't your plugin, so the issue is resolved. >.<

For people running into a similar issue as I did, where you set the tiles (which works) but the update and fitting of the "ruletile" does not work; Maybe you are using floats.

In my generation method I read a JSON file where the settings are in. Like which tile has what noise_value, tile_id, layer_id, and all that. JSON only knows "numbers". Neither int nor float. So Godot parses all of those as float. _update_tile_tiles than checked if [5] has 5 and returned false. Because int 5 is not float 5. Which is absolutely correct.

I have a logic now in place, which makes sure that I actually hand over the tile_id as an int. image

Thank you really much for your help!

Portponky commented 6 months ago

Ah, classic JSON.

Processing via a changeset on other threads is still worth implementing if you are changing large numbers of tiles, as it'll prevent the main thread from blocking and affecting fps.

Good luck with your game!

griessbreilp commented 6 months ago

Yes, I do that now! :) Reduced my whole code from 280 lines down to 170, absolutely worth it + massive performance increase^^

Thank you really much, and keep up your awesome work! <3