Skip to content
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.

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

Auto create tile for multiple atlases #79678

Conversation

thiagola92
Copy link
Contributor

@thiagola92 thiagola92 commented Jul 19, 2023

Based on issues: #69273 #79473
And PRs: #70790 #79558

  • Support for auto create tiles when adding multiple atlases
  • Button to add Atlas can be used for multiple files
  • Unify functions TileSetEditor::_texture_file_selected() and TileSetEditor::_drop_data_fw() into TileSetEditor::_load_texture_files()

Bugsquad edit:

@groud
Copy link
Member

groud commented Jul 20, 2023

Thanks for the contribution!

Why do you create 3 functions to do handle that, and store atlases as a property of the editor ? I would suggest to instead use a single function that takes an array instead, it would be simpler.

@Chaosus
Copy link
Member

Chaosus commented Jul 20, 2023

And commits needs to be squashed in order for PR to be accepted (see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html).

@Chaosus Chaosus added this to the 4.2 milestone Jul 20, 2023
@thiagola92
Copy link
Contributor Author

@groud I really didn't want to store atlases as a property but I don't know how to get the array after the user accept/decline a popup.

void TileSetAtlasSourceEditor::init_source(Vector<Ref<TileSetAtlasSource>> atlases) {
	tool_setup_atlas_source_button->set_pressed(true);
	atlases_to_auto_create_tiles = atlases;
	confirm_auto_create_tiles->popup_centered();
}

_clear_auto_create_tiles() created for when the user cancel the popup.
_auto_create_tiles_in_atlas() i think that i'm going to move the logic back to _auto_create_tiles()
Can you tell me the third function? init_source() existed before to deal with the popup.

@Chaosus Thank you, I will read and do right after any adjustment from feedbacks

@groud
Copy link
Member

groud commented Jul 20, 2023

@groud I really didn't want to store atlases as a property but I don't know how to get the array after the user accept/decline a popup.

Ah I see what you mean. I think it's fine to do it that way then, using a property. However I would probably move the definition closer to the popup's one in the .h file, so that's it's a bit more obvious this variable is only needed for the popup.

@thiagola92
Copy link
Contributor Author

@groud Like this?

void _auto_create_tiles();
void _auto_remove_tiles();
void _cancel_auto_create_tiles();
AcceptDialog *confirm_auto_create_tiles = nullptr;

Also I renamed _clear_auto_create_tiles() to _cancel_auto_create_tiles().

@groud
Copy link
Member

groud commented Jul 20, 2023

I meant something like this:

void _auto_create_tiles();
void _auto_remove_tiles();
void _cancel_auto_create_tiles();
AcceptDialog *confirm_auto_create_tiles = nullptr;
Vector<Ref<TileSetAtlasSource>> atlases_to_auto_create_tiles;

Related variables and methods close together.

@thiagola92 thiagola92 force-pushed the auto_create_tilesets_for_multiple_atlas branch from cb94347 to b5a2ef6 Compare July 20, 2023 16:04
@thiagola92
Copy link
Contributor Author

@groud Sorry, fixed.
@Chaosus Squashed.

@thiagola92 thiagola92 force-pushed the auto_create_tilesets_for_multiple_atlas branch from 7df17a3 to 914fad6 Compare July 24, 2023 08:58
@thiagola92 thiagola92 force-pushed the auto_create_tilesets_for_multiple_atlas branch from 7dfb184 to 914fad6 Compare August 1, 2023 20:53
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Needs a rebase, but I believe looks mostly good.

editor/plugins/tiles/tile_set_editor.cpp Outdated Show resolved Hide resolved
@thiagola92 thiagola92 force-pushed the auto_create_tilesets_for_multiple_atlas branch 2 times, most recently from fb631b7 to 9678e73 Compare August 3, 2023 20:17
@thiagola92
Copy link
Contributor Author

Moved the comment // Handle dropping a texture in the list of atlas resources. back to _drop_data_fw()

@thiagola92 thiagola92 force-pushed the auto_create_tilesets_for_multiple_atlas branch from 9678e73 to d331837 Compare August 18, 2023 06:50
@thiagola92
Copy link
Contributor Author

@groud Can you confirm if i did rebase correctly?

@thiagola92 thiagola92 force-pushed the auto_create_tilesets_for_multiple_atlas branch 2 times, most recently from 4c2be06 to 33db6dc Compare August 18, 2023 09:40
@thiagola92
Copy link
Contributor Author

@AThousandShips thank you for reviewing.
Changed as requested.

@thiagola92 thiagola92 force-pushed the auto_create_tilesets_for_multiple_atlas branch from 33db6dc to 541fd0f Compare August 27, 2023 12:40
TileSet add button support multiple files
Join most of the code of `_drop_data_fw()` and `_texture_file_selected()` in a new function `_load_texture_files()`
Rename `init_source` to `init_new_atlases`
@thiagola92 thiagola92 force-pushed the auto_create_tilesets_for_multiple_atlas branch from 541fd0f to c8a94ea Compare August 27, 2023 13:45
@akien-mga akien-mga merged commit a7ded90 into godotengine:master Aug 28, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@thiagola92 thiagola92 deleted the auto_create_tilesets_for_multiple_atlas branch September 7, 2023 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants