Reduce TileMapLayerEditor's undo/redo memory usage

[chocola-mint]

Fixes #107853

This PR heavily reduces the amount of undo/redo memory used by TileMapLayerEditor with two optimizations.

• Edits that have no effect (cell is same before and after) are not recorded in EditorUndoRedoManager.
  • Mostly helps with redundant operations like clicking the bucket tool on the same tile multiple times.
• Edits are compressed into rectangular regions (TileMapLayerEditor::Rect) before being recorded in EditorUndoRedoManager.
  • Helps with bucket and rect tools, whose edits tend to cover large, contiguous regions.

Improvement is immediately obvious with the bucket tool, but other tools can also benefit from this optimization to different degrees.

This PR now only eliminates redundant edits.

[groud]

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.

[KoBeWi]

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).

[chocola-mint]

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.

[groud]

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.

[chocola-mint]

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

[groud]

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.

[chocola-mint]

@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.

[groud]

To avoid unneeded copy:

Suggested change

	void TileMapLayerSubEditorPlugin::_add_to_output_if_tile_changed(HashMap<Vector2i, TileMapCell> &p_output, const TileMapLayer *p_layer, Vector2i p_coords, TileMapCell p_cell) {
	void TileMapLayerSubEditorPlugin::_add_to_output_if_tile_changed(HashMap<Vector2i, TileMapCell> &p_output, const TileMapLayer *p_layer, const Vector2i &p_coords, const TileMapCell &p_cell) {

[chocola-mint]

Vector2i and TileMapCell are both merely 8 bytes. Passing by copy is likely going to be cheaper than passing by reference.

(It's like, you wouldn't normally use const int&, or const uint64_t& right?)

[groud]

I guess it's more for consistency TBH (and I guess future-proofing in case we increase those). But anyway, up to you, both work I guess.

[AThousandShips]

We don't do it for simple types (int, bool, etc.) but we generally do for complex types because they might have constructors etc., but the result depends on how the original variable is stored, if it's a temporary it's probably cheaper to pass by copy, but if it's a local value there's an additional dereference and copy when passing by value rather than by reference

[groud]

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!