Skip to content

Reduce TileMapLayerEditor's undo/redo memory usage #107969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+21 −14
Open

Reduce TileMapLayerEditor's undo/redo memory usage #107969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions editor/plugins/tiles/tile_map_layer_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ TileMapLayer *TileMapLayerSubEditorPlugin::_get_edited_layer() const {
return ObjectDB::get_instance<TileMapLayer>(edited_tile_map_layer_id);
}

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

@groud groud Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@chocola-mint chocola-mint Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (p_cell != p_layer->get_cell(p_coords)) {
p_output[p_coords] = p_cell;
}
}

void TileMapLayerSubEditorPlugin::draw_tile_coords_over_viewport(Control *p_overlay, const TileMapLayer *p_edited_layer, Ref<TileSet> p_tile_set, bool p_show_rectangle_size, const Vector2i &p_rectangle_origin) {
Point2 msgpos = Point2(20 * EDSCALE, p_overlay->get_size().y - 20 * EDSCALE);
String text = String(p_tile_set->local_to_map(p_edited_layer->get_local_mouse_position()));
Expand Down Expand Up @@ -1074,7 +1080,7 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTilesPlugin::_draw_line(Vector2
// Paint a random tile.
Vector<Vector2i> line = TileMapLayerEditor::get_line(edited_layer, tile_set->local_to_map(p_from_mouse_pos), tile_set->local_to_map(p_to_mouse_pos));
for (int i = 0; i < line.size(); i++) {
output.insert(line[i], _pick_random_tile(pattern));
_add_to_output_if_tile_changed(output, edited_layer, line[i], _pick_random_tile(pattern));
}
} else {
// Paint the pattern.
Expand All @@ -1091,7 +1097,7 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTilesPlugin::_draw_line(Vector2
Vector2i top_left = line[i] * pattern->get_size() + offset;
for (int j = 0; j < used_cells.size(); j++) {
Vector2i coords = tile_set->map_pattern(top_left, used_cells[j], pattern);
output.insert(coords, TileMapCell(pattern->get_cell_source_id(used_cells[j]), pattern->get_cell_atlas_coords(used_cells[j]), pattern->get_cell_alternative_tile(used_cells[j])));
_add_to_output_if_tile_changed(output, edited_layer, coords, TileMapCell(pattern->get_cell_source_id(used_cells[j]), pattern->get_cell_atlas_coords(used_cells[j]), pattern->get_cell_alternative_tile(used_cells[j])));
}
}
}
Expand Down Expand Up @@ -1132,7 +1138,7 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTilesPlugin::_draw_rect(Vector2
for (int x = 0; x < rect.size.x; x++) {
for (int y = 0; y < rect.size.y; y++) {
Vector2i coords = rect.position + Vector2i(x, y);
output.insert(coords, _pick_random_tile(pattern));
_add_to_output_if_tile_changed(output, edited_layer, coords, _pick_random_tile(pattern));
}
}
} else {
Expand All @@ -1144,7 +1150,7 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTilesPlugin::_draw_rect(Vector2
for (int j = 0; j < used_cells.size(); j++) {
Vector2i coords = pattern_coords + used_cells[j];
if (rect.has_point(coords)) {
output.insert(coords, TileMapCell(pattern->get_cell_source_id(used_cells[j]), pattern->get_cell_atlas_coords(used_cells[j]), pattern->get_cell_alternative_tile(used_cells[j])));
_add_to_output_if_tile_changed(output, edited_layer, coords, TileMapCell(pattern->get_cell_source_id(used_cells[j]), pattern->get_cell_atlas_coords(used_cells[j]), pattern->get_cell_alternative_tile(used_cells[j])));
}
}
}
Expand Down Expand Up @@ -1195,16 +1201,16 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTilesPlugin::_draw_bucket_fill(
(source_cell.source_id != TileSet::INVALID_SOURCE || boundaries.has_point(coords))) {
if (!p_erase && random_tile_toggle->is_pressed()) {
// Paint a random tile.
output.insert(coords, _pick_random_tile(pattern));
_add_to_output_if_tile_changed(output, edited_layer, coords, _pick_random_tile(pattern));
} else {
// Paint the pattern.
Vector2i pattern_coords = (coords - p_coords) % pattern->get_size(); // Note: it would be good to have posmodv for Vector2i.
pattern_coords.x = pattern_coords.x < 0 ? pattern_coords.x + pattern->get_size().x : pattern_coords.x;
pattern_coords.y = pattern_coords.y < 0 ? pattern_coords.y + pattern->get_size().y : pattern_coords.y;
if (pattern->has_cell(pattern_coords)) {
output.insert(coords, TileMapCell(pattern->get_cell_source_id(pattern_coords), pattern->get_cell_atlas_coords(pattern_coords), pattern->get_cell_alternative_tile(pattern_coords)));
_add_to_output_if_tile_changed(output, edited_layer, coords, TileMapCell(pattern->get_cell_source_id(pattern_coords), pattern->get_cell_atlas_coords(pattern_coords), pattern->get_cell_alternative_tile(pattern_coords)));
} else {
output.insert(coords, TileMapCell());
_add_to_output_if_tile_changed(output, edited_layer, coords, TileMapCell());
}
}

Expand Down Expand Up @@ -1241,16 +1247,16 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTilesPlugin::_draw_bucket_fill(
(source_cell.source_id != TileSet::INVALID_SOURCE || boundaries.has_point(coords))) {
if (!p_erase && random_tile_toggle->is_pressed()) {
// Paint a random tile.
output.insert(coords, _pick_random_tile(pattern));
_add_to_output_if_tile_changed(output, edited_layer, coords, _pick_random_tile(pattern));
} else {
// Paint the pattern.
Vector2i pattern_coords = (coords - p_coords) % pattern->get_size(); // Note: it would be good to have posmodv for Vector2i.
pattern_coords.x = pattern_coords.x < 0 ? pattern_coords.x + pattern->get_size().x : pattern_coords.x;
pattern_coords.y = pattern_coords.y < 0 ? pattern_coords.y + pattern->get_size().y : pattern_coords.y;
if (pattern->has_cell(pattern_coords)) {
output.insert(coords, TileMapCell(pattern->get_cell_source_id(pattern_coords), pattern->get_cell_atlas_coords(pattern_coords), pattern->get_cell_alternative_tile(pattern_coords)));
_add_to_output_if_tile_changed(output, edited_layer, coords, TileMapCell(pattern->get_cell_source_id(pattern_coords), pattern->get_cell_atlas_coords(pattern_coords), pattern->get_cell_alternative_tile(pattern_coords)));
} else {
output.insert(coords, TileMapCell());
_add_to_output_if_tile_changed(output, edited_layer, coords, TileMapCell());
}
}
}
Expand Down Expand Up @@ -2533,7 +2539,7 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTerrainsPlugin::_draw_terrain_p
for (const KeyValue<Vector2i, TileSet::TerrainsPattern> &kv : terrain_fill_output) {
if (painted_set.has(kv.key)) {
// Paint a random tile with the correct terrain for the painted path.
output[kv.key] = tile_set->get_random_tile_from_terrains_pattern(p_terrain_set, kv.value);
_add_to_output_if_tile_changed(output, edited_layer, kv.key, tile_set->get_random_tile_from_terrains_pattern(p_terrain_set, kv.value));
} else {
// Avoids updating the painted path from the output if the new pattern is the same as before.
TileSet::TerrainsPattern in_map_terrain_pattern = TileSet::TerrainsPattern(*tile_set, p_terrain_set);
Expand All @@ -2550,7 +2556,7 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTerrainsPlugin::_draw_terrain_p
}
}
if (in_map_terrain_pattern != kv.value) {
output[kv.key] = tile_set->get_random_tile_from_terrains_pattern(p_terrain_set, kv.value);
_add_to_output_if_tile_changed(output, edited_layer, kv.key, tile_set->get_random_tile_from_terrains_pattern(p_terrain_set, kv.value));
}
}
}
Expand Down Expand Up @@ -2580,7 +2586,7 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTerrainsPlugin::_draw_terrain_p
for (const KeyValue<Vector2i, TileSet::TerrainsPattern> &kv : terrain_fill_output) {
if (painted_set.has(kv.key)) {
// Paint a random tile with the correct terrain for the painted path.
output[kv.key] = tile_set->get_random_tile_from_terrains_pattern(p_terrain_set, kv.value);
_add_to_output_if_tile_changed(output, edited_layer, kv.key, tile_set->get_random_tile_from_terrains_pattern(p_terrain_set, kv.value));
} else {
// Avoids updating the painted path from the output if the new pattern is the same as before.
TileSet::TerrainsPattern in_map_terrain_pattern = TileSet::TerrainsPattern(*tile_set, p_terrain_set);
Expand All @@ -2597,7 +2603,7 @@ HashMap<Vector2i, TileMapCell> TileMapLayerEditorTerrainsPlugin::_draw_terrain_p
}
}
if (in_map_terrain_pattern != kv.value) {
output[kv.key] = tile_set->get_random_tile_from_terrains_pattern(p_terrain_set, kv.value);
_add_to_output_if_tile_changed(output, edited_layer, kv.key, tile_set->get_random_tile_from_terrains_pattern(p_terrain_set, kv.value));
}
}
}
Expand Down
1 change: 1 addition & 0 deletions editor/plugins/tiles/tile_map_layer_editor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class TileMapLayerSubEditorPlugin : public Object {
protected:
ObjectID edited_tile_map_layer_id;
TileMapLayer *_get_edited_layer() const;
static void _add_to_output_if_tile_changed(HashMap<Vector2i, TileMapCell> &p_output, const TileMapLayer *p_layer, Vector2i p_coords, TileMapCell p_cell);

public:
struct TabData {
Expand Down