Add a way to know when a TileMapLayer's cell is modified

[Khusheete]

Hi !

This PR adds the signal cell_changed to the TileMapLayer. It allows for making tools that react to modifications to the TileMapLayer's cells.

This feature was discussed in godotengine/godot-proposals#668. Back in godot 3.2, #27500 made so the TileMapEditor called TileMap.set_cell through Object.call, this allowed the function to be overridden in GDScript or C# in @tool scripts. However with the tile map reworks, this was removed.

I decided not to reimplement the fix from #27500 as having a signal seemed simpler while having the same capabilities.

[SaracenOne]

I think this does make sense as a callback. There's certain types of automation tools which automatically place relevant tiles based on the context of surrounding tiles, but such tools require a callback like this.

[Mickeon]

We lightly discussed in the past (@groud), and we generally agreed that it's not a good idea as is.

This will be emitted every time any tile change occurs. However, to make it brief, emitting the signal multiple times for many, many changes adds up, even if the signal is not connected to anything. Tile maps are not exactly known for their performance as they are now, either.
I may be completely wrong on this, but this will even be emitted on initialization, for every single existing cell.

Despite the concerns, this feature is very much desired. I myself would need this, as I heavily relied on overriding set_cell in 3.x. What I was somewhat thinking is as follows:

• Store the modified cell. This is already done by marking the cell as dirty, see the surrounding code.
• When they are all updated in bulk, emit a single signal containing an array of cells that have been modified.

One alternative is to make this signal only available in the editor. It can be argued that while editing the TileMap the performance hit is pretty minor.

Another alternative is to find a way to toggle the emission of this signal but... I wouldn't know how to feel about that.

[SaracenOne]

The issue raised with overloading the signal does seem valid, yes. It's tricky to handle this in a way that won't seem like overengineering things, but a batched solution like you suggested might be desirable. Another potential suggestion I'll offer is that you could make it so that emitting a signal dependent on whether an explicit mode is set in the TileMapLayer, hopefully removing any unwanted overhead for users who don't require such a callback. While not a signal, there is some precedent in the engine already with NOTIFICATION_TRANFORM_CHANGED which requires the explicit calling of set_notify_transform(), which removes the overhead when its not needed as opposed to making it an explicit editor vs not-editor thing.

[Mickeon]

While not a signal, there is some precedent in the engine already with NOTIFICATION_TRANFORM_CHANGED

Indeed. And that is not a signal for... roughly the same reasons. Maybe demoting this to a virtual function would be enough, but the risk of excessive overhead is still there. Some benchmark tests could be nice.

[Khusheete]

Mmh.. Right, I do agree, having the signal here is not a great idea. I understaind how trying to emit the signal would impact performance negatively, and the function actually seems to be called a few times per cell on init. If we add to that the fact that modifying the tile that has just been changed through the signal would require it to be called once more, that is probably a bit much.

A batched solution would definitly be better, as it solves most of the issues. Such a signal/callback would still be called multiple times if tile modifications are made within it.

Would it be better to have the solution inside the TileMapLayerEditor ? (like in #27500)
Having the TileMapLayerEditor asks some component (either the TileMapLayer itself or some other thing - eg. a TileMapSomethingPlugin) before modifying the TileMapLayer now seems to me like a better choice: it would most likely perform better, and it would prevent issues with having functions called several times.

Was there a particular reason why #27500 was not kept in 4.0 ?

[Mickeon]

If we add to that the fact that modifying the tile that has just been changed through the signal would require it to be called once more, that is probably a bit much.

Actually, this would cause a feedback loop and crash the application, unless it is checked manually. It also happens when handling NOTIFICATION_TRANFORM_CHANGED & similar, so perhaps this implementation detail is inevitable. The batched solution wouldn't have this issue, indeed.

Would it be better to have the solution inside the TileMapLayerEditor ?

It's a good suggestion, although TileMapLayerEditor is not exposed to scripting languages so it may not matter too much. Though I can see the desire in having this work at run-time, as well.

Was there a particular reason why #27500 was not kept in 4.0 ?

Being able to override non-virtual methods has always been quirky. Whether it actually works as imagined depends purely on whether the internal code calls the method directly (with pointers), or through Object.call(). In Godot 4.0 most calls are done directly, which is blazingly fast in comparison, but it also means the calls do not go through the overridden methods in a custom script.
I hope I explained it correctly.

[Khusheete]

Actually, this would cause a feedback loop and crash the application, unless it is checked manually. It also happens when handling NOTIFICATION_TRANFORM_CHANGED & similar, so perhaps this implementation detail is inevitable. The batched solution wouldn't have this issue, indeed.

It is not that bad because it is checked internally (for what I assume are performance reasons). So as long as the tile that the user wants to place does not change between calls, there should not be any feedback loop. However, if the user ever decide to use randomness in the setting of the tile (to make use of tile alternatives for example), there is a chance of a feedback loop crashing godot.

I hope I explained it correctly.

I think you did, thanks :).

Maybe there could be two functions to set a cell then: a "checked" and an "unchecked" one.
The "unchecked" version is the current TileMapLayer.set_cell. The "checked" one would try to call a virtual _set_cell function that can be overridden in a scripting language (giving the same functionnality as #27500). If it is not overridden, then the "checked" set_cell would do the same thing as the "unchecked" one.

This would work both in editor and at runtime because the user can call the "checked" version if they want. And as a (small) bonus, it would still be possible to call the "unchecked" version from a script if needed for performance.

[groud]

Hi, thanks for the contribution.
As mentioned by others, I don't think this is a good idea. And, in general, we avoid adding signals that are direct consequences of the user's action (like, here, you need to call set_cell on the TileMap, we would avoid generate a signal right away as the user can simply call another function right away).

We could add this to the editor, but it might not be easy to do, as there are likely several places where this is called, and it does not handle use cases for in-game situations.

That being said, I think one thing that could be done is adding a callback in the TileMapLayer when cells are updated. Updated are deferred (to avoid too many updates), and they work with a "group of cells to update" being provided. So we could have a virtual method called:
_update_cells(PackedVector2iArray p_array)

[Khusheete]

And, in general, we avoid adding signals that are direct consequences of the user's action (like, here, you need to call set_cell on the TileMap, we would avoid generate a signal right away as the user can simply call another function right away).

Alright, good to know ! I think it makes sense as well.

That being said, I think one thing that could be done is adding a callback in the TileMapLayer when cells are updated. Updated are deferred (to avoid too many updates), and they work with a "group of cells to update" being provided. So we could have a virtual method called:
_update_cells(PackedVector2iArray p_array)

Yup, that could work. It would still be called mutliple times when trying to use set_cell inside _update_cells to put constrains on cells or to have a custom autotile, which would seem to be the most common usecase of such a feature. However the impact is far smaller than having separate callbacks for each cell modification, so maybe it's the way to go regardless.

Maybe there could be two functions to set a cell then: a "checked" and an "unchecked" one.
The "unchecked" version is the current TileMapLayer.set_cell. The "checked" one would try to call a virtual _set_cell function that can be overridden in a scripting language (giving the same functionnality as #27500). If it is not overridden, then the "checked" set_cell would do the same thing as the "unchecked" one.

If I'm not mistaken, this would not have any of the issues raised so far, as it is called before any modification to the tilemap. In addition, the new function would be called only by the TileMapLayerEditor and by the user when they need it, so it would probably not hinder performances too much.
Though maybe it should be batched. Some use cases could benefit from it (I know I could make use of that). Then the callback could look something like: _set_cells_checked(PackedVector2iArray coords, PackedInt64Array source_id, PackedVector2iArray atlas_coords, PackedInt64Array alternative_tiles) (I don't really like this name: I don't feel that it properly describes what it does).

Should I try some of the ideas and see what sticks ? They don't seem to require a lot of work to have in a working state, so that might help decide on what is best.
If I do so, should I link the experiments here or should it be done elsewhere ?
Also, should this PR be closed and replaced by another one when a solution has been reached, should it be commited in this PR ?

[groud]

Yup, that could work. It would still be called mutliple times when trying to use set_cell inside _update_cells to put constrains on cells or to have a custom autotile, which would seem to be the most common usecase of such a feature. However the impact is far smaller than having separate callbacks for each cell modification, so maybe it's the way to go regardless.

No. This function is guaranteed to be only run once per frame. So if you call set_cell in that function, the update will be ran in the next frame. I believe it's an acceptable tradeoff though, to avoid infinite updates.

As a side note, I'd really avoid making the API more complex than it is right now. I'd rather avoid adding more argument to set_cell, or new functions it's hard to understand what they do at a glance.

[AThousandShips]

You pushed your branch in such a way that it cleared any changes, which automatically closes the PR, to reset use git reset --hard 1aef7cf and try again

[Khusheete]

Ooops ! Yes I did something wrong, I was about to work on @Mickeon's and @groud's idea and wanted to revert the previous commits.
Should I reset to get back those commits ? We did all agree that what was initially done was not a good idea, and that something else had to be done.

[AThousandShips]

I'd suggest opening a new PR with a separate branch so you avoid issues with the master branch, good to start from scratch

[Khusheete]

Ok, thanks !
When doing the new PR, I'll link that one to have an easy access to this discussion.

[boholstapp]

This is how I use it
https://www.youtube.com/watch?v=7_fwNiZL0xY&t=59s