Hey, thanks for the contribution. While I think the added check is welcome, I think this PR is too much code for what it is trying to solve. I prefer we waste more memory than needed than adding almost 200 lines of code to the editor.

That being said, I think we can at least keep the change about preventing a cell to be added if it does not cause any change. I'd suggest we keep the approach where you wrap the check in a macro (could be a function too), to conditionally add things to undo_redo. Also, note that you should avoid allocating new arrays like you did, this can harm performances.

That being said, if you absolutely want this optimization to be done. I think a better approach would be to see how TileMapLayer data is serialized into an array to be stored as a property via set_tile_map_data_from_array / get_tile_map_data_as_array. This code can probably be modified to support partial updates, and thus used instead to update cells in batches. It was already optimized to pack TileMapData in as little memory as possible so it could word pretty well.

btw this reminds me of this comment: #60836 (comment)
We support reverse ops now, so some code can be potentially simplified (not necessarily related to this PR though).

I prefer we waste more memory than needed than adding almost 200 lines of code to the editor.

While I agree on principle, it's still a lot of memory. And I believe the implementation isn't some unsalvageable spaghetti code and we can make it maintainable enough for the editor team to be comfortable with it.

Also, note that you should avoid allocating new arrays like you did, this can harm performances.

I think the amount of memory saved here far outweighs the performance loss of allocating like, 4 additional temporary Vectors.

Do keep in mind that adding methods to EditorUndoRedoManager is in itself an allocation and there is an allocation for every set_data added to EditorUndoRedoManager, so it's still overall a plus.

And if the temporary Vectors are really that much of a concern, we could make them member variables of TileMapLayerEditor and be even more optimized with our memory usage.

That being said, if you absolutely want this optimization to be done. I think a better approach would be to see how TileMapLayer data is serialized into an array to be stored as a property via set_tile_map_data_from_array / get_tile_map_data_as_array. This code can probably be modified to support partial updates, and thus used instead to update cells in batches. It was already optimized to pack TileMapData in as little memory as possible so it could word pretty well.

I think these are different use cases.

TileMapLayer's serialized format is just a tightly-packed but uncompressed bitstream, and it's fine because it's used in runtime, load time is a priority, and the compression rate for an actual compression algorithm would be pretty shaky anyway.

TileMapLayerEditor's operations however have properties that make them have better compression rates even with just run-length encoding. As mentioned, they usually result in edits that cover contiguous regions of same tiles.

While I agree on principle, it's still a lot of memory. And I believe the implementation isn't some unsalvageable spaghetti code and we can make it maintainable enough for the editor team to be comfortable with it.

I mean, sorry, but the TileMap editor is already full of these kind of special optimizations here and there, for cases that do matter a lot more as they make the editor unusable otherwise. While I agree it can be a lot of memory, the added complexity here is not an acceptable tradeoff to me, considering how complex the editor already is. It would be fine if the TileMap editor wasn't that complex though, I agree the code is likely fine enough in itself. Just not in this context.

I think the amount of memory saved here far outweighs the performance loss of allocating like, 4 additional temporary Vectors.

4 temporary vectors that could could contains thousands of tiles. If it can be avoided, it's better. But yeah, I do agree it's already stored as a vector under the hood anyway, so it could be fine.

TileMapLayer's serialized format is just a tightly-packed but uncompressed bitstream, and it's fine because it's used in runtime, load time is a priority, and the compression rate for an actual compression algorithm would be pretty shaky anyway.

I mean, I am not sure I understand here. Your implementation does not really add compression either. And both have the objective to store things more or less as compact as possible. So I think to avoid impacting the codebase too much, this is an acceptable tradeoff.

Your implementation does not really add compression either. And both have the objective to store things more or less as compact as possible. So I think to avoid impacting the codebase too much, this is an acceptable tradeoff.

Yes it does. It merges single-tile edits into rectangular edits. That's compression. It's a form of run-length encoding in 2D.

Which is to say, an edit that turns this:

XXXX
XXXX
XXXX

Into this:

OOOX
OOOX
OOOO
OOOO

Is compressed into two commands (pseudocode for brevity) and submitted to EditorUndoRedoManager:

DRAW_RECT((0, 0), (3, 4), O)
DRAW_RECT((3, 2), (1, 2), O)

DRAW_RECT((0, 0), (3, 4), O) draws the O in the following:

OOOX
OOOX
OOOX
OOOX

And DRAW_RECT((3, 2), (1, 2), O) draws the O in the following:

XXXX
XXXX
XXXO
XXXO

Ok I see. I still don't think it's worth the complexity, but I do agree in the rect and the bucket-contiguous operations it would help reducing the memory usage.

@groud

I've removed everything else aside from the redundant edit elimination, and I've folded the macro into a function and made TileMapLayerEditorTerrainsPlugin use it as well.

Hopefully this is simple enough to be acceptable.

Aside from the small nitpick above and CI not passing (looks unrelated to your change though, so you might just have to rebase on latest master), it looks good to me!

Rebased and decided to go with const TileMapCell & in the end.

Thanks!