Added TileMap::set_cell_region() and TileMap::set_tile_data()

[ben-humphries]

This PR is trying to improve on and generalize the PR by @zzz-assault here: #38324

I think it is also worth reading through this issue:
#31020

TileMap::set_cell_region() should have the benefits of the set_line() performance improvements while also being useful in a more general case.

TileMap::set_tile_data() is simply revealing to the user the TileMap::_set_tile_data() function. I think this could be very useful in the case where large amounts of tile data need to be updated quickly, or asynchronously, or in place of TileMap::set_cell_region() when flip_h, flip_v, transpose, or autotile_coords need to be specified. The reason I looked into this topic is that a game I'm working on would benefit from asynchronously updating a large TileMap. TileMap is not thread-safe, but populating an array of tile data could be made thread-safe, and then this function could be called once the threads are complete.

I think the TileMap::set_tile_data() function would be more useful if the data format was well documented. I've included comments in this PR, and I will paste a function from GDScript here that can be used to convert individual cell info into a packed data format:

func pack_cell_data(x : int, y : int, id : int, flip_h : bool = false, flip_v : bool = false, transpose : bool = false, autotile_x : int = 0, autotile_y : int = 0):
	
	y <<= 16
	var coord_packed = x + y
	
	var id_packed = id
	id_packed |= int(flip_h) << 29
	id_packed |= int(flip_v) << 30
	id_packed |= int(transpose) << 31
	
	autotile_y <<= 16
	var autotile_packed = autotile_x + autotile_y
	
	return PackedInt32Array([coord_packed, id_packed, autotile_packed])

Here is a small project that demonstrates the use of both functions:
TilemapTest.zip

This is my first PR to Godot, I'm sorry if something is formatted incorrectly or I've gone through the process the wrong way.

[zzz-assault]

Hi,
Glad you are taking this issue and providing a solution.
Need to check the next days :)

[akien-mga]

Style issue here, the brace should be on the same line as the prototype (see https://docs.godotengine.org/en/latest/community/contributing/code_style_guidelines.html).

[groud]

I am not sure about exposing the set_tile_data function, it's quite a cryptic and complex API as is.
It might be ok if we don't have a better solution but that's annoys me a little bit.

[ben-humphries]

Sorry about the formatting, that should now be fixed. Good suggestion on Rect2i, I hadn't known about that previously.

@groud As for exposing the set_tile_data() function, I am also torn because it isn't a user friendly format, but I think it could be if there was good documentation to go along with it. Without directly uploading data, I'm not sure how best to change a massive amount of tiles, especially if you need to include the extra variables like flip_h or autotile coords.

Then again, this property can just be accessed using the set() and get() functions from GDScript. Maybe it should stay that way?

[zzz-assault]

@ben-humphries Hi, sorry for late feedback, wanted to test it now, but I'm not sure how "const Vector &p_ids" must be filled. Could you please explain the logic shortly?

@ all -> using set_tile_data function means skipping some Tilemap internals, like "Quadrant" setup ... I would not recommend to expose it to godot users (if really needed an experienced user can still use set("tile_data", array)), got some weird behaviour in my former experiments (like unusual high memory usage, crashes combined with Y-sort, etc.)

Just asking, but why is a tanspose, flip, etc. really needed as mass change? I would assume that if mass data change (via code) is needed, there is anyhow either bitmasks used or in total no flipping, transposing needed. So I would keep this functions for the single cell changes and skip them in the mass change, by either set it them to false or by taking over the setup of the cell which should be changed (this would make the mass change easier to code AND easier for the users to understand).

[ben-humphries]

@zzz-assault It's been a while since I've worked on this, but I think you may have misunderstood my intention -- the set_tile_data() function has to include transpose, flip, etc. but a user could make any kind of "packing" function they would like. A packing function could have those variables automatically filled.

[zzz-assault]

@ben-humphries Yes I know, but what I meant is, that for your added function set_cell_region() there is no need to include the flips / transpose ... and due to the issues I see with set_tile_data I would simply not expose it.

But could you please explain shortly how to correctly load the ID's (const Vector &p_ids) in GD-Script to use your function? (just wanted to test the function and performance gain).

[ben-humphries]

@zzz-assault Oh, I see. The vector p_ids is a one dimensional list representing a 2D area -- so x and y coordinates in the region would have the following relation:

id at (x,y) = p_ids[y * p_region.size.width + x]

[zzz-assault]

Looks good, but I'm struggling with the one dimensional array for IDs, I think this adds complexity to the function, which might be a hurdle for the usual godot user (although it might be better for performance).

I would prefer a ivec3 array, as this is easy to understand and the performance impact shouldn't be huge (I hope, not tested yet). This would also eliminate the need of an irect and allows the biggest flexibility, as cells could be skipped if no change is needed as all cells are directly addressed with coordinate and id.

[YuriSizov]

@groud Is this still relevant after the rework?

[groud]

@groud Is this still relevant after the rework?

More or less. Now that all properties for tiles that were in the TileMap node were moved to the TileSet resource, I would be easier to implement such a function, setting a bunch of tiles from an array.

However, I still don't think such function would make a significant performance difference, as it's simply a matter of moving a few for loops from gdscript to C++. This is similar to what is proposed godotengine/godot-proposals#764. With a similar setup, I showed that moving some loops from GDscript to C++ barely improved performance by 1% there.

The only issue this could solve is if we have a huge overhead by setting up tiles one by one instead of a batch (like if this triggers internal updates or something like that). But in that case, I'd rather have a boolean to disable such updates than providing a batch function that would require building a GDScript array to work. Also, keep in mind building such an array has a performance cost, which could limit even more a potential performance gain.

[akien-mga]

Given the discussion on godotengine/godot-proposals#1538 and above, it's not clear whether this would actually:

• Improve performance in the way users expect it, as this seems to be the main motivation for the change
• Actually provide a facility which is flexible enough for the use cases it should support

So after discussing with @groud we suggest going back to the drawing board for now and continuing the discussion in godotengine/godot-proposals#1538 to really narrow down on a consensual change that would address a well defined (and benchmarked) problem.