Cleanup tiles outside the texture #77986

KoBeWi · 2023-06-07T23:15:14Z

groud · 2023-06-08T07:55:44Z

Hmm, sorry but I don't think this is the correct way to solve the bug. The whole API was meant to automatically remove tiles that are outside the texture (basically, if you reduce the texture size, tiles get deleted). You can have a look to TileSetAtlasSource::_clear_tiles_outside_texture that is called in set_texture

So the bug should likely be instead of fixing it that way.

KoBeWi · 2023-06-08T09:00:26Z

This bug happens when a texture is updated externally. I thought that user might want to avoid data loss, hence the option.

I'll try to make it automatic. Still, there could be some corner cases where you might end up with invalid outside tiles and right now there is no way to remove them other than editing the tileset manually (which might be binary). I think the option is useful anyway.

groud · 2023-06-08T09:19:16Z

I'll try to make it automatic. Still, there could be some corner cases where you might end up with invalid outside tiles and right now there is no way to remove them other than editing the tileset manually (which might be binary). I think the option is useful anyway.

I guess we should connect to the changed signal of Texture for that case. Theoretically it should work.

I'd rather not show the option if we can, a "fix bug" button isn't really the best IMO... But I guess, that, if this happens again for whatever reason, we should probably run this function internally from time to time (like when opening the atlas editor or something like that).

KoBeWi · 2023-06-10T23:36:53Z

Ok I removed the option. The editor will do a sweep of invalid tiles when setting tileset + it connects to texture's changed signal.

groud · 2023-06-12T07:58:32Z

Ah, that's was not what I really meant, sorry. Part of this should likely be done in the TileSetAtlasSource code itself, not in the editor. A bit like how it is already done to updated the padded texture:

void TileSetAtlasSource::set_texture(Ref<Texture2D> p_texture) {
	if (texture.is_valid()) {
		texture->disconnect(SNAME("changed"), callable_mp(this, &TileSetAtlasSource::_queue_update_padded_texture));
	}

	texture = p_texture;

	if (texture.is_valid()) {
		texture->connect(SNAME("changed"), callable_mp(this, &TileSetAtlasSource::_queue_update_padded_texture));
	}

	_clear_tiles_outside_texture();
	_queue_update_padded_texture();
	emit_changed();
}

That being said, it likely would make it quite difficult for the editor to save the tiles before the texture is reduced in size (for undo-redo). Which might be a big problem. I could manage to save tiles before deletion when the source itself was modified, but for sub-resources, I have no clue if that is possible.

So, in the end, I wonder if we should not modify the editor so that an atlas can have tiles outside of it's texture, and users would be able to clean them manually. But that's a lot of work...

I guess I can only see those solutions:

accept the bug and add the button.
Maybe find a way for the editor to detect the texture is about to be reduced in size, and save the tiles accordingly (I don't know how to implement this be we might be able to hack something)
Allows tiles outside the texture: probably the cleanest but likely a lot of work.

KoBeWi · 2023-06-12T08:58:44Z

Part of this should likely be done in the TileSetAtlasSource code itself, not in the editor.

But the tile cleanup is a costly operation and the problem is editor-only.

groud · 2023-06-12T09:12:46Z

But the tile cleanup is a costly operation and the problem is editor-only.

Not really. If you modify the texture in game code, the tiles outside the texture will still be there, while they are not really supposed to. That's why the set_texture code itself I pasted above does include the call to _clear_tiles_outside_texture.
It's mostly visible in the editor, but the inconsistency is still there at runtime.

I am thinking that maybe a solution would be the following:

Remove all calls to _clear_tiles_outside_texture() in code, then expose it as public method.
Add an has_tile_outside_texture() public method
Warn the user somehow in the editor, that the atlas has tiles outside the texture (by calling has_tile_outside_texture), and make it easy to click a button to cleanup the tiles outside the texture. A bit like you originally did. However, this should support undo/redo.
Cleanup _undo_redo_inspector_callback in TileSetAtlasSourceEditor so that they don't save tiles outside the texture anymore, as it would not be needed. The get_tiles_to_be_removed_on_change function could likely be used before calling clear_tiles_outside_texture in the editor to save the tiles to be removed.

KoBeWi · 2023-07-14T00:48:51Z

Done requested changes. The user is notified by the toaster:

godot.windows.editor.dev.x86_64_MpMikqsP0Q.mp4

I didn't do the _undo_redo_inspector_callback() changes as I didn't quite get what you meant.

YuriSizov · 2023-07-25T16:29:16Z

cc @groud could you give it a review?

groud · 2023-07-26T14:53:12Z

The message should not be spawned by the Toaster, but displayed as a warning sign with a tooltip somewhere in the atlas editor (maybe in the toolbar ?). Because the toaster messages will disappear after a little while.

Cleanup _undo_redo_inspector_callback in TileSetAtlasSourceEditor so that they don't save tiles outside the texture anymore, as it would not be needed. The get_tiles_to_be_removed_on_change function could likely be used before calling clear_tiles_outside_texture in the editor to save the tiles to be removed.

Part of the role of TileSetAtlasSourceEditor::_undo_redo_inspector_callback is to save things to avoid data loss in case of as value change. Without your PR, changing texture, margins, separation or region_size does cause some tiles to be removed automatically (with calls to _clear_tiles_outside_texture). TileSetAtlasSourceEditor::_undo_redo_inspector_callback simply saves the tile data of the deleted tiles so that using ctrl+z brings them back.

With your changes, _clear_tiles_outside_texture isn't called anymore. As a consequence, we can remove the logic saving the tiles data in TileSetAtlasSourceEditor::_undo_redo_inspector_callback. It should improve performance a bit by not saving useless data.

JVMV · 2023-08-04T22:18:40Z

so whats up? will it merged?

YuriSizov · 2023-08-05T00:01:46Z

@Isenther If it is approved. See the comment directly above yours.

KoBeWi · 2023-08-05T11:31:35Z

The message should not be spawned by the Toaster, but displayed as a warning sign with a tooltip somewhere in the atlas editor (maybe in the toolbar ?). Because the toaster messages will disappear after a little while.

You mean the warning should be displayed permanently until you clear the tiles?

As a consequence, we can remove the logic saving the tiles data in TileSetAtlasSourceEditor::_undo_redo_inspector_callback

Not sure which code do you mean exactly.

groud · 2023-08-22T11:05:59Z

Not sure which code do you mean exactly.

This can be removed:

	TileSetAtlasSourceProxyObject *atlas_source_proxy = Object::cast_to<TileSetAtlasSourceProxyObject>(p_edited);
	if (atlas_source_proxy) {
		Ref<TileSetAtlasSource> atlas_source = atlas_source_proxy->get_edited();
		ERR_FAIL_COND(!atlas_source.is_valid());
		UndoRedo *internal_undo_redo = undo_redo_man->get_history_for_object(atlas_source.ptr()).undo_redo;
		internal_undo_redo->start_force_keep_in_merge_ends();
		PackedVector2Array arr;
		if (p_property == "texture") {
			arr = atlas_source->get_tiles_to_be_removed_on_change(p_new_value, atlas_source->get_margins(), atlas_source->get_separation(), atlas_source->get_texture_region_size());
		} else if (p_property == "margins") {
			arr = atlas_source->get_tiles_to_be_removed_on_change(atlas_source->get_texture(), p_new_value, atlas_source->get_separation(), atlas_source->get_texture_region_size());
		} else if (p_property == "separation") {
			arr = atlas_source->get_tiles_to_be_removed_on_change(atlas_source->get_texture(), atlas_source->get_margins(), p_new_value, atlas_source->get_texture_region_size());
		} else if (p_property == "texture_region_size") {
			arr = atlas_source->get_tiles_to_be_removed_on_change(atlas_source->get_texture(), atlas_source->get_margins(), atlas_source->get_separation(), p_new_value);
		}
		if (!arr.is_empty()) {
			// Get all properties assigned to a tile.
			List<PropertyInfo> properties;
			atlas_source->get_property_list(&properties);
			for (int i = 0; i < arr.size(); i++) {
				Vector2i coords = arr[i];
				String prefix = vformat("%d:%d/", coords.x, coords.y);
				for (PropertyInfo pi : properties) {
					if (pi.name.begins_with(prefix)) {
						ADD_UNDO(atlas_source.ptr(), pi.name);
					}
				}
			}
		}
		internal_undo_redo->end_force_keep_in_merge_ends();
	}

KoBeWi · 2023-08-22T12:03:35Z

What about my other question? >_>

groud · 2023-08-22T12:25:03Z

You mean the warning should be displayed permanently until you clear the tiles?

Yes

KoBeWi · 2023-08-26T15:18:21Z

doc/classes/TileSetAtlasSource.xml

editor/plugins/tiles/tile_set_atlas_source_editor.cpp

akien-mga · 2023-08-28T11:18:40Z

editor/plugins/tiles/tile_set_atlas_source_editor.cpp

 	tool_advanced_menu_button->get_popup()->connect("id_pressed", callable_mp(this, &TileSetAtlasSourceEditor::_menu_option));
 	tool_settings->add_child(tool_advanced_menu_button);

+	outside_tiles_warning_label = memnew(Label);
+	outside_tiles_warning_label->set_text(TTR("The current atlas source has tiles outside the texture.") + " (?)");
+	outside_tiles_warning_label->set_tooltip_text(TTR("You can clear it using \"Remove Tiles Outside the Texture\" option in 3 dots menu."));


I think it's technically called a "kebab menu", but I don't know how widespread this term is for neophytes. So "3 dots" might be fine.

Suggested change

outside_tiles_warning_label->set_tooltip_text(TTR("You can clear it using \"Remove Tiles Outside the Texture\" option in 3 dots menu."));

outside_tiles_warning_label->set_tooltip_text(TTR("You can clear it using the \"Remove Tiles Outside the Texture\" option in the 3 dots menu."));

Could possibly also do this if we want to make sure translators don't mistakenly translate this differently in the menu and in the tooltip. But on the other hand it makes the sentence slightly harder to understand when translating.

Suggested change

outside_tiles_warning_label->set_tooltip_text(TTR("You can clear it using \"Remove Tiles Outside the Texture\" option in 3 dots menu."));

outside_tiles_warning_label->set_tooltip_text(vformat(TTR("You can clear it using the \"%s\" option in the 3 dots menu."), TTR("Remove Tiles Outside the Texture")));

I think it's technically called a "kebab menu", but I don't know how widespread this term is for neophytes. So "3 dots" might be fine.

There is a vertical ellipsis unicode character: ⋮
I could use it here.

Sounds good.

Doesn't work unfortunately :<

You need to use U"..." for Unicode strings.

I also did the vformat change for the option name, but it still has a risk of the text not being updated if the option is renamed.

editor/plugins/tiles/tile_set_atlas_source_editor.cpp

groud · 2023-08-28T11:26:58Z

I think this takes a bit too much space IMO. Sorry I was not clear, but my idea was to simply display a yellow warning icon in the toolbar, then use the tooltip to display the error message when hovering the icon.

KoBeWi · 2023-08-28T11:28:48Z

At first I tried using the full text, but then moved half of it into the tooltip. Now it's actually small.
But yeah, icon makes sense.

KoBeWi · 2023-08-28T13:15:50Z

akien-mga · 2023-08-28T18:38:42Z

Thanks!

KoBeWi added enhancement topic:editor topic:2d labels Jun 7, 2023

KoBeWi added this to the 4.x milestone Jun 7, 2023

akien-mga requested a review from groud June 8, 2023 06:39

KoBeWi force-pushed the outside_the_texture,_only_death_awaits branch 2 times, most recently from bfde195 to 846695e Compare June 10, 2023 23:35

KoBeWi changed the title ~~Add option to cleanup tiles outside the texture~~ Cleanup tiles outside the texture Jun 10, 2023

KoBeWi force-pushed the outside_the_texture,_only_death_awaits branch from 846695e to 50185ad Compare July 14, 2023 00:47

KoBeWi force-pushed the outside_the_texture,_only_death_awaits branch from 50185ad to 996ae81 Compare July 14, 2023 21:28

KoBeWi requested a review from a team as a code owner July 14, 2023 21:28

YuriSizov modified the milestones: 4.x, 4.2 Jul 17, 2023

KoBeWi force-pushed the outside_the_texture,_only_death_awaits branch 2 times, most recently from 7ad3662 to 4b685df Compare July 18, 2023 11:20

KoBeWi force-pushed the outside_the_texture,_only_death_awaits branch from 4b685df to 52588aa Compare August 26, 2023 15:18

akien-mga reviewed Aug 28, 2023

View reviewed changes

doc/classes/TileSetAtlasSource.xml Outdated Show resolved Hide resolved

akien-mga reviewed Aug 28, 2023

View reviewed changes

editor/plugins/tiles/tile_set_atlas_source_editor.cpp Outdated Show resolved Hide resolved

akien-mga reviewed Aug 28, 2023

View reviewed changes

AThousandShips reviewed Aug 28, 2023

View reviewed changes

editor/plugins/tiles/tile_set_atlas_source_editor.cpp Outdated Show resolved Hide resolved

Add option to cleanup tiles outside the texture

52d41cc

KoBeWi force-pushed the outside_the_texture,_only_death_awaits branch from 52588aa to 52d41cc Compare August 28, 2023 13:15

groud approved these changes Aug 28, 2023

View reviewed changes

akien-mga merged commit 9229ea1 into godotengine:master Aug 28, 2023
15 checks passed

KoBeWi deleted the outside_the_texture,_only_death_awaits branch August 28, 2023 19:34

Lunarexxy mentioned this pull request Dec 2, 2023

Tileset tiles outside texture cannot be removed #85661

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup tiles outside the texture #77986

Cleanup tiles outside the texture #77986

KoBeWi commented Jun 7, 2023

groud commented Jun 8, 2023

KoBeWi commented Jun 8, 2023

groud commented Jun 8, 2023

KoBeWi commented Jun 10, 2023

groud commented Jun 12, 2023

KoBeWi commented Jun 12, 2023

groud commented Jun 12, 2023

KoBeWi commented Jul 14, 2023

YuriSizov commented Jul 25, 2023

groud commented Jul 26, 2023

JVMV commented Aug 4, 2023

YuriSizov commented Aug 5, 2023 •

edited

Loading

KoBeWi commented Aug 5, 2023

groud commented Aug 22, 2023

KoBeWi commented Aug 22, 2023

groud commented Aug 22, 2023

KoBeWi commented Aug 26, 2023

akien-mga Aug 28, 2023 •

edited

Loading

akien-mga Aug 28, 2023

KoBeWi Aug 28, 2023

akien-mga Aug 28, 2023

KoBeWi Aug 28, 2023

akien-mga Aug 28, 2023 •

edited

Loading

KoBeWi Aug 28, 2023

groud commented Aug 28, 2023

KoBeWi commented Aug 28, 2023

KoBeWi commented Aug 28, 2023

akien-mga commented Aug 28, 2023

	outside_tiles_warning_label->set_tooltip_text(TTR("You can clear it using \"Remove Tiles Outside the Texture\" option in 3 dots menu."));
	outside_tiles_warning_label->set_tooltip_text(TTR("You can clear it using the \"Remove Tiles Outside the Texture\" option in the 3 dots menu."));

Cleanup tiles outside the texture #77986

Cleanup tiles outside the texture #77986

Conversation

KoBeWi commented Jun 7, 2023

groud commented Jun 8, 2023

KoBeWi commented Jun 8, 2023

groud commented Jun 8, 2023

KoBeWi commented Jun 10, 2023

groud commented Jun 12, 2023

KoBeWi commented Jun 12, 2023

groud commented Jun 12, 2023

KoBeWi commented Jul 14, 2023

YuriSizov commented Jul 25, 2023

groud commented Jul 26, 2023

JVMV commented Aug 4, 2023

YuriSizov commented Aug 5, 2023 • edited Loading

KoBeWi commented Aug 5, 2023

groud commented Aug 22, 2023

KoBeWi commented Aug 22, 2023

groud commented Aug 22, 2023

KoBeWi commented Aug 26, 2023

akien-mga Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

akien-mga Aug 28, 2023

Choose a reason for hiding this comment

KoBeWi Aug 28, 2023

Choose a reason for hiding this comment

akien-mga Aug 28, 2023

Choose a reason for hiding this comment

KoBeWi Aug 28, 2023

Choose a reason for hiding this comment

akien-mga Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

KoBeWi Aug 28, 2023

Choose a reason for hiding this comment

groud commented Aug 28, 2023

KoBeWi commented Aug 28, 2023

KoBeWi commented Aug 28, 2023

akien-mga commented Aug 28, 2023

YuriSizov commented Aug 5, 2023 •

edited

Loading

akien-mga Aug 28, 2023 •

edited

Loading

akien-mga Aug 28, 2023 •

edited

Loading