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

Add support for creating tiles when adding multiple atlases to a tileset #70790

Conversation

jamesmintram
Copy link
Contributor

Adds support for "batch" creating tiles when dragging multiple atlases into a tileset. See: #69273

@jamesmintram jamesmintram changed the title [WIPAdd support for creating tiles when adding multiple atlases to a tileset [WIP] Add support for creating tiles when adding multiple atlases to a tileset Jan 1, 2023

void TileSetAtlasSourceEditor::_auto_create_tiles(Vector<Ref<TileSetAtlasSource>> const &new_tile_set_atlas_sources) {

Ref<EditorUndoRedoManager> &undo_redo = EditorNode::get_undo_redo();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Single undo scope for all tile generation

@jamesmintram jamesmintram force-pushed the jamesm/feat/auto-create-tilesets-for-multiple-on-drop branch from 6a9b1ef to 3fd152b Compare January 1, 2023 01:35
@@ -259,7 +259,9 @@ class TileSetAtlasSourceEditor : public HSplitContainer {
void _unhandled_key_input(const Ref<InputEvent> &p_event);

// -- Misc --
void _auto_create_tiles(Vector<Ref<TileSetAtlasSource>> const &new_tile_set_atlas_sources);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better naming?

Copy link
Member

Choose a reason for hiding this comment

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

Should be const Vector<...> &, and the parameter should start with p_.

@Calinou Calinou added this to the 4.0 milestone Jan 1, 2023
@jamesmintram jamesmintram force-pushed the jamesm/feat/auto-create-tilesets-for-multiple-on-drop branch from 3fd152b to a6e2d49 Compare January 1, 2023 12:07
@jamesmintram jamesmintram marked this pull request as ready for review January 1, 2023 12:09
@jamesmintram jamesmintram changed the title [WIP] Add support for creating tiles when adding multiple atlases to a tileset Add support for creating tiles when adding multiple atlases to a tileset Jan 1, 2023
@jamesmintram jamesmintram force-pushed the jamesm/feat/auto-create-tilesets-for-multiple-on-drop branch from 72375e2 to 5350237 Compare January 1, 2023 12:15
@jamesmintram
Copy link
Contributor Author

Video of the changes in action, note the Undo error that appears is a pre-existing issue and (probably) out of scope for this change:

batch-add-tileset-atlas

@jamesmintram jamesmintram force-pushed the jamesm/feat/auto-create-tilesets-for-multiple-on-drop branch from 5350237 to 82fa415 Compare January 1, 2023 12:34
@@ -119,6 +119,8 @@ class TileSetAtlasSourceEditor : public HSplitContainer {

bool tile_set_changed_needs_update = false;

Vector<Ref<TileSetAtlasSource>> pendingAtlases;
Copy link
Member

Choose a reason for hiding this comment

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

Please use snake_case for identifiers. You might see from surrounding code that we never use camelCase ;)

@@ -275,7 +279,7 @@ class TileSetAtlasSourceEditor : public HSplitContainer {

public:
void edit(Ref<TileSet> p_tile_set, TileSetAtlasSource *p_tile_set_source, int p_source_id);
void init_source();
void init_sources(Vector<Ref<TileSetAtlasSource>> newSources);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void init_sources(Vector<Ref<TileSetAtlasSource>> newSources);
void init_sources(Vector<Ref<TileSetAtlasSource>> p_new_sources);

@akien-mga akien-mga modified the milestones: 4.0, 4.x Jan 9, 2023
@groud
Copy link
Member

groud commented Jan 9, 2023

I think the idea is fine. The implementation looks correct enough, but as the popup and automatic creation of tiles would not depends anymore on the currently edited atlas source, the logic to create those tiles should be moved outside of TileSetAtlasSourceEditor. Could you move all this code to TileSetEditor instead ?

@coppolaemilio
Copy link
Member

Hello @jamesmintram, thanks for making the PR! Would you be up to make the latest requested changes or do you want someone else to take over?


Fixes #69273

@akien-mga
Copy link
Member

Superseded by #79678.

@akien-mga akien-mga closed this Aug 28, 2023
@AThousandShips AThousandShips removed this from the 4.x milestone Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants